OLS-3130: Operator-managed OKP / Solr hybrid RAG via RHOKP sidecar#1653
OLS-3130: Operator-managed OKP / Solr hybrid RAG via RHOKP sidecar#1653blublinsky wants to merge 1 commit into
Conversation
|
/retest |
1 similar comment
|
/retest |
blublinsky
force-pushed
the
okp-support
branch
6 times, most recently
from
b6e8679 to
09d7a94
Compare
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds RHOKP sidecar support and Solr hybrid RAG configuration gated by ChangesRHOKP and Solr hybrid RAG
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant cmd_main as cmd/main.go
participant reconciler as OLSConfigReconciler
participant assets as buildOLSConfig
participant deploy as GenerateOLSDeployment
cmd_main->>cmd_main: read --rhokp-image
cmd_main->>reconciler: set RHOOKPImage option
reconciler->>assets: buildOLSConfig(cr)
assets->>assets: emit reference indexes + solr_hybrid
reconciler->>deploy: GenerateOLSDeployment(cr)
deploy->>deploy: add RHOKP sidecar when !byokRAGOnly
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
blublinsky
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@blublinsky: This pull request references OLS-3130 which is a valid jira issue. 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. |
![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 (9)
.ai/spec/how/config-generation.md (1)
71-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReference index example uses incorrect key names.
The
reference_content.indexesexample showspath:,index_id:,origin:, but the actual generated keys (perReferenceIndexjson tags) areproduct_docs_index_path,product_docs_index_id,product_docs_origin. Every other section of this doc uses literal generated key names — this block is inconsistent and could mislead a reader debugging real config output.🤖 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 @.ai/spec/how/config-generation.md around lines 71 - 83, The reference_content.indexes example uses placeholder key names that don’t match the generated config fields. Update the example to use the actual ReferenceIndex JSON tag names from the generated output, namely product_docs_index_path, product_docs_index_id, and product_docs_origin, and keep the rest of the block aligned with the concrete generated keys used elsewhere in config-generation.md.internal/controller/appserver/assets.go (1)
296-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge the two consecutive identical-condition
ifblocks.Lines 298 and 306 both guard on
!cr.Spec.OLSConfig.ByokRAGOnly. Splitting them obscures that theByokIndexflag applies to all reference indexes (including user BYOK entries added earlier), not just the OCP index just appended. Combine them into a single block with a comment explaining why all entries get flagged.♻️ Proposed refactor
if !cr.Spec.OLSConfig.ByokRAGOnly { ocpReferenceIndex := utils.ReferenceIndex{ ProductDocsIndexPath: "/app-root/vector_db/ocp_product_docs/" + r.GetOpenShiftMajor() + "." + r.GetOpenshiftMinor(), ProductDocsIndexId: "ocp-product-docs-" + r.GetOpenShiftMajor() + "_" + r.GetOpenshiftMinor(), ProductDocsOrigin: "Red Hat OpenShift " + r.GetOpenShiftMajor() + "." + r.GetOpenshiftMinor() + " documentation", } referenceIndexes = append(referenceIndexes, ocpReferenceIndex) - } - if !cr.Spec.OLSConfig.ByokRAGOnly { + + // lightspeed-service requires byok_index on every FAISS index (BYOK and OCP) when solr_hybrid is active. for i := range referenceIndexes { referenceIndexes[i].ByokIndex = true } }🤖 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 `@internal/controller/appserver/assets.go` around lines 296 - 310, The appserver reference index setup has two separate `if !cr.Spec.OLSConfig.ByokRAGOnly` blocks in `assets.go`, which hides that `ByokIndex` must be applied to every entry in `referenceIndexes`, not just the newly added OCP index. Merge the OCP index append and the `ByokIndex` loop into one conditional block in `referenceIndexes` handling, and add a short comment clarifying that the flag is set for all reference indexes, including any user BYOK entries already present.internal/controller/appserver/assets_test.go (1)
129-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelf-referential
SolrHybridassertions don't test actual default values.These three assertions compare the generated config's
SolrHybridagainstbuildSolrHybridSettings()— the very function under test — so they'll pass even if the function's field values silently regress. The test at Line 618-635 in this same file already does this correctly by asserting against explicitutils.SolrHybridXXXDefaultconstants; apply the same pattern here instead of calling the production function as the expected value.As per path instructions, "Reject tests that only mirror implementation without asserting behavior or reconciliation outcomes."
Also applies to: 1595-1615, 1629-1646
🤖 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 `@internal/controller/appserver/assets_test.go` around lines 129 - 133, The SolrHybrid assertions are self-referential because they use buildSolrHybridSettings() as the expected value, so the test can’t catch regressions in default fields. Update the affected assertions in assets_test.go to compare the generated config against explicit utility default constants, following the existing pattern used elsewhere in this file with utils.SolrHybridXXXDefault values instead of calling buildSolrHybridSettings() as the expectation.Source: Path instructions
internal/controller/utils/constants.go (2)
485-486: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winFloating
:latesttag as a coded default image.
RHOOKPImageDefaultpins to:latest. The comment notes it's meant to be overridden via--rhokp-imageuntil the image is productized, but if the flag is ever omitted the operator will silently pull whateverlatestresolves to at runtime, which is a reproducibility/drift risk for a "default".🤖 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 `@internal/controller/utils/constants.go` around lines 485 - 486, The default image constant RHOOKPImageDefault is using a floating :latest tag, which can drift at runtime if --rhokp-image is not provided. Update the default in constants.go to use a fixed, immutable image reference instead of latest, and keep the surrounding RHOKP-related logic consistent so callers of RHOOKPImageDefault and any image selection code continue to work with the pinned value.
372-403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNaming inconsistency:
RHOOKP(double-O) vs. product nameRHOKP.Every new identifier here (
RHOOKPHTTPPort,RHOOKPContainerName,RHOOKPImageDefault, etc.) spells the acronym with two O's, but the actual literal values, the PR title, and the commit message all useRHOKP(single O) — Red Hat's official product name for the Offline Knowledge Portal. For example, Line 458 assignsRHOOKPContainerName = "rhokp", mismatching the identifier and its own value. This typo is replicated acrosstypes.go,interface.go, andolsconfig_helpers.go(and likelyrhokp.go/cmd/main.goin later cohorts). Worth a global rename now before this spreads further into the codebase and CLI/flag surface.♻️ Example rename (repeat across all affected files)
- RHOOKPHTTPPort = 9080 - RHOOKPHTTPSPort = 9443 + RHOKPHTTPPort = 9080 + RHOKPHTTPSPort = 9443 ... - RHOOKPContainerName = "rhokp" + RHOKPContainerName = "rhokp"Also applies to: 457-458, 485-486
🤖 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 `@internal/controller/utils/constants.go` around lines 372 - 403, The new constants and related identifiers use an inconsistent acronym spelling, with RHOOKP appearing instead of the product-correct RHOKP. Rename the affected symbols in this constants block and across the referenced usages so the identifier names consistently match the official product name, including items like RHOOKPHTTPPort, RHOOKPContainerName, and RHOOKPImageDefault-style constants. Update all references in the related helpers and interface/type files together to avoid breaking the public surface or leaving mixed naming behind.internal/controller/appserver/deployment.go (1)
543-543: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment no longer matches the gating condition.
The comment says the RHOKP sidecar is added "if Solr hybrid RAG is configured," but the actual condition (line 596) is
!cr.Spec.OLSConfig.ByokRAGOnly— there's no separate "Solr hybrid RAG configured" toggle. Minor doc drift that could confuse future readers.📝 Suggested comment fix
- // 3. RHOKP Solr sidecar (if Solr hybrid RAG is configured) + // 3. RHOKP Solr sidecar (added unless ByokRAGOnly is true)🤖 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 `@internal/controller/appserver/deployment.go` at line 543, Update the comment near the RHOKP Solr sidecar logic in deployment.go so it matches the actual gating in the deployment path driven by `cr.Spec.OLSConfig.ByokRAGOnly`. The current “if Solr hybrid RAG is configured” wording is misleading; rephrase it to describe the real condition used by the surrounding deployment code, keeping the comment aligned with the `RHOKP` sidecar handling and the `ByokRAGOnly` check.internal/controller/appserver/deployment_test.go (1)
650-741: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving RHOKP sidecar tests out of the "Proxy settings" Context.
These two new
Itblocks aren't related to proxy configuration; placing them insideContext("Proxy settings", ...)means each one incurs the unrelatedBeforeEach/AfterEachsetup/teardown of the provider secret,additional-ca/proxy CA configmap, and OpenShift certs configmap. A dedicatedContext("RHOKP sidecar", ...)(or top-levelDescribe) would make intent clearer and avoid unnecessary fixture churn.🤖 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 `@internal/controller/appserver/deployment_test.go` around lines 650 - 741, The new RHOKP sidecar specs are misplaced inside the Proxy settings context in deployment_test.go, so move the two It blocks that validate GenerateOLSDeployment, RHOOKPContainerName, and OCPClusterVersionEnvVar into a dedicated RHOKP sidecar Context or top-level Describe. Keep the proxy-related tests under the existing Proxy settings context, and place the RHOKP assertions with their own setup so they no longer inherit the provider secret, additional-ca, proxy CA, and OpenShift cert fixture setup/teardown.internal/controller/appserver/rhokp.go (2)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfusing comment: "mel" is unclear.
The doc comment twice references "mel" (
before mel start,mel's httpd pre-init), which doesn't read as intended English and obscures the important port-remap rationale documented here.✏️ Suggested comment fix
-// rhokpStartupScript remaps RHOKP Apache HTTP/HTTPS listen ports before mel start so MCP (8080) +// rhokpStartupScript remaps RHOKP Apache HTTP/HTTPS listen ports before startup so MCP (8080) // and the app server (8443) can keep the stock ports in the shared pod network namespace. -// ssl.conf must remain present — mel's httpd pre-init reads it for TLS cert setup. +// ssl.conf must remain present — the RHOKP image's httpd pre-init reads it for TLS cert setup.🤖 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 `@internal/controller/appserver/rhokp.go` around lines 11 - 13, The doc comment on rhokpStartupScript uses “mel” in two places, which is unclear and makes the port-remap rationale harder to understand. Update the comment text in rhokpStartupScript to use the intended product/process name consistently and read naturally, while preserving the explanation that the startup script remaps Apache HTTP/HTTPS ports so MCP and the app server can keep their stock ports and ssl.conf must remain present for TLS cert setup.
14-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent RHOKP casing across helpers in the same file.
rhokpStartupScript,rhokpContainerCommand, andrhokpContainerArgsuse lowercase "rhokp" whilegenerateRHOOKPEnv(and all importedutils.RHOOKP*constants) use "RHOOKP" (double O). This inconsistency within a single file is easy to mistype and confusing during future maintenance.✏️ Suggested rename for consistency
-func rhokpStartupScript() string { +func rhookpStartupScript() string { ... -func rhokpContainerCommand() []string { +func rhookpContainerCommand() []string { ... -func rhokpContainerArgs() []string { +func rhookpContainerArgs() []string {🤖 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 `@internal/controller/appserver/rhokp.go` around lines 14 - 52, Rename the helper functions in this file to use the same RHOKP casing as the existing utils.RHOOKP* constants and generateRHOOKPEnv, since the current mix of rhokp and RHOOKP is inconsistent. Update the identifiers rhokpStartupScript, rhokpContainerCommand, and rhokpContainerArgs to the same RHOKP-style naming used elsewhere, and make sure any internal calls between these helpers are updated together so the file stays consistent and easy to maintain.
🤖 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 `@internal/controller/appserver/rhokp.go`:
- Around line 15-25: The command built in the rhokp helper is matching numeric
prefixes too broadly in its sed replacements. Update the formatting in the
function that returns the shell command so the replacements for the Listen and
_default_ directives are anchored to the full line by adding an end-of-line
boundary, preventing accidental rewrites of values like 80800 or 84430. Use the
existing rhokp command construction symbols and the related utils port/conf path
constants to locate the affected replacements.
In `@internal/controller/appserver/suite_test.go`:
- Around line 196-217: The helper functions deploymentContainerCount and
expectedAppServerEnv are closing over the package-level cr instead of the
per-test cr used inside the App server deployment tests. Update these helpers to
accept the relevant OLSConfig/CR as an explicit argument, and change all call
sites in deployment_test.go to pass the local cr from each BeforeEach/It block
so ByokRAGOnly is evaluated against the correct test state.
---
Nitpick comments:
In @.ai/spec/how/config-generation.md:
- Around line 71-83: The reference_content.indexes example uses placeholder key
names that don’t match the generated config fields. Update the example to use
the actual ReferenceIndex JSON tag names from the generated output, namely
product_docs_index_path, product_docs_index_id, and product_docs_origin, and
keep the rest of the block aligned with the concrete generated keys used
elsewhere in config-generation.md.
In `@internal/controller/appserver/assets_test.go`:
- Around line 129-133: The SolrHybrid assertions are self-referential because
they use buildSolrHybridSettings() as the expected value, so the test can’t
catch regressions in default fields. Update the affected assertions in
assets_test.go to compare the generated config against explicit utility default
constants, following the existing pattern used elsewhere in this file with
utils.SolrHybridXXXDefault values instead of calling buildSolrHybridSettings()
as the expectation.
In `@internal/controller/appserver/assets.go`:
- Around line 296-310: The appserver reference index setup has two separate `if
!cr.Spec.OLSConfig.ByokRAGOnly` blocks in `assets.go`, which hides that
`ByokIndex` must be applied to every entry in `referenceIndexes`, not just the
newly added OCP index. Merge the OCP index append and the `ByokIndex` loop into
one conditional block in `referenceIndexes` handling, and add a short comment
clarifying that the flag is set for all reference indexes, including any user
BYOK entries already present.
In `@internal/controller/appserver/deployment_test.go`:
- Around line 650-741: The new RHOKP sidecar specs are misplaced inside the
Proxy settings context in deployment_test.go, so move the two It blocks that
validate GenerateOLSDeployment, RHOOKPContainerName, and OCPClusterVersionEnvVar
into a dedicated RHOKP sidecar Context or top-level Describe. Keep the
proxy-related tests under the existing Proxy settings context, and place the
RHOKP assertions with their own setup so they no longer inherit the provider
secret, additional-ca, proxy CA, and OpenShift cert fixture setup/teardown.
In `@internal/controller/appserver/deployment.go`:
- Line 543: Update the comment near the RHOKP Solr sidecar logic in
deployment.go so it matches the actual gating in the deployment path driven by
`cr.Spec.OLSConfig.ByokRAGOnly`. The current “if Solr hybrid RAG is configured”
wording is misleading; rephrase it to describe the real condition used by the
surrounding deployment code, keeping the comment aligned with the `RHOKP`
sidecar handling and the `ByokRAGOnly` check.
In `@internal/controller/appserver/rhokp.go`:
- Around line 11-13: The doc comment on rhokpStartupScript uses “mel” in two
places, which is unclear and makes the port-remap rationale harder to
understand. Update the comment text in rhokpStartupScript to use the intended
product/process name consistently and read naturally, while preserving the
explanation that the startup script remaps Apache HTTP/HTTPS ports so MCP and
the app server can keep their stock ports and ssl.conf must remain present for
TLS cert setup.
- Around line 14-52: Rename the helper functions in this file to use the same
RHOKP casing as the existing utils.RHOOKP* constants and generateRHOOKPEnv,
since the current mix of rhokp and RHOOKP is inconsistent. Update the
identifiers rhokpStartupScript, rhokpContainerCommand, and rhokpContainerArgs to
the same RHOKP-style naming used elsewhere, and make sure any internal calls
between these helpers are updated together so the file stays consistent and easy
to maintain.
In `@internal/controller/utils/constants.go`:
- Around line 485-486: The default image constant RHOOKPImageDefault is using a
floating :latest tag, which can drift at runtime if --rhokp-image is not
provided. Update the default in constants.go to use a fixed, immutable image
reference instead of latest, and keep the surrounding RHOKP-related logic
consistent so callers of RHOOKPImageDefault and any image selection code
continue to work with the pinned value.
- Around line 372-403: The new constants and related identifiers use an
inconsistent acronym spelling, with RHOOKP appearing instead of the
product-correct RHOKP. Rename the affected symbols in this constants block and
across the referenced usages so the identifier names consistently match the
official product name, including items like RHOOKPHTTPPort, RHOOKPContainerName,
and RHOOKPImageDefault-style constants. Update all references in the related
helpers and interface/type files together to avoid breaking the public surface
or leaving mixed naming behind.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 89d46389-0839-4011-a1b1-74ba3b11fd02
⛔ Files ignored due to path filters (2)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated.deepcopy.goconfig/crd/bases/ols.openshift.io_olsconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (26)
.ai/spec/how/config-generation.md.ai/spec/how/deployment-generation.md.ai/spec/what/app-server.md.ai/spec/what/crd-api.md.gitignoreMakefileapi/v1alpha1/olsconfig_types.gocmd/main.goconfig/default/deployment-patch.yamlconfig/dev/metrics-reader-token.yamlhack/image_placeholders.jsoninternal/controller/appserver/assets.gointernal/controller/appserver/assets_test.gointernal/controller/appserver/deployment.gointernal/controller/appserver/deployment_test.gointernal/controller/appserver/reconciler_test.gointernal/controller/appserver/rhokp.gointernal/controller/appserver/rhokp_test.gointernal/controller/appserver/suite_test.gointernal/controller/olsconfig_helpers.gointernal/controller/reconciler/interface.gointernal/controller/utils/constants.gointernal/controller/utils/resource_defaults_test.gointernal/controller/utils/testing.gointernal/controller/utils/types.gointernal/controller/utils/utils.go
| return fmt.Sprintf( | ||
| `sed -i 's/^Listen %d/Listen %d/' %s && `+ | ||
| `sed -i 's/^Listen 0.0.0.0:%d https/Listen 0.0.0.0:%d https/' %s && `+ | ||
| `sed -i 's/_default_:%d/_default_:%d/' %s && `+ | ||
| `exec %s %s`, | ||
| utils.RHOOKPImageHTTPPort, utils.RHOOKPHTTPPort, utils.RHOOKPHTTPDConfPath, | ||
| utils.RHOOKPImageHTTPSPort, utils.RHOOKPHTTPSPort, utils.RHOOKPHTTPDSSLConfPath, | ||
| utils.RHOOKPImageHTTPSPort, utils.RHOOKPHTTPSPort, utils.RHOOKPHTTPDSSLConfPath, | ||
| utils.RHOOKPContainerEntrypoint, | ||
| utils.RHOOKPMainCommand, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Red Hat OKP RHOKP image default httpd.conf Listen port configuration
💡 Result:
The Red Hat Offline Knowledge Portal (RHOKP), which is built upon the Red Hat UBI 9 httpd 2.4 container image, uses port 8080 as its default HTTP listening port and port 8443 as its default HTTPS listening port [1][2][3]. This configuration is standard for Red Hat's Apache HTTP Server container images (such as those in the SCLorg/httpd-container ecosystem) to ensure compatibility with OpenShift Container Platform security constraints, which typically restrict containers from binding to reserved ports below 1024 [4][5][6][7]. While older versions of some httpd images historically defaulted to port 80, modern RHEL-based httpd images have shifted to port 8080 to support non-root execution [8]. When deploying RHOKP or similar httpd-based workloads in OpenShift, you must configure your Services and Routes to map traffic to these target ports [9][10]. For example, when launching the RHOKP container, the standard port mapping is 8080:8080 and 8443:8443 [1][3][11].
Citations:
- 1: https://docs.redhat.com/en/documentation/red_hat_offline_knowledge_portal/1/html/making_red_hat_expert_knowledge_available_in_offline_environments/proc_installing-rhokp_deploying-rhokp
- 2: https://docs.redhat.com/en/documentation/red_hat_offline_knowledge_portal/1/html-single/making_red_hat_expert_knowledge_available_in_offline_environments/making_red_hat_expert_knowledge_available_in_offline_environments
- 3: https://blog.hofstede.it/red-hat-offline-knowledge-portal-all-the-docs-air-gapped-on-your-laptop/
- 4: https://stackoverflow.com/questions/55950776/how-to-deploy-apache-server-in-openshift
- 5: https://docs.redhat.com/en/documentation/openshift_container_platform/2/html/user_guide/sect-binding_applications_to_ports
- 6: https://github.com/sclorg/httpd-container
- 7: https://sclorg.github.io/containers-doc/httpd/2.4/
- 8: https://docs.redhat.com/en/documentation/red_hat_software_collections/3/html/using_red_hat_software_collections_container_images/daemon-images
- 9: https://gist.github.com/dmc5179/406712bfc4ec2164bfe3257223bef985
- 10: https://developers.redhat.com/articles/2025/10/03/how-deploy-offline-knowledge-portal-openshift
- 11: rludva/support@af9a4be
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and search RHOKP-related references.
git ls-files | rg '^internal/controller/appserver/|rhokp|RHOOKP|RHOKP|httpd\.conf|ssl\.conf|Listen '
echo '--- outline: internal/controller/appserver/rhokp.go ---'
ast-grep outline internal/controller/appserver/rhokp.go --view expanded || true
echo '--- surrounding file lines ---'
cat -n internal/controller/appserver/rhokp.go | sed -n '1,120p'
echo '--- search for RHOKP constants and command usage ---'
rg -n "RHOOKP|RHOKP|httpd\.conf|ssl\.conf|Listen 8080|Listen 8443|_default_" internal/controller cmd docs bundle -S || trueRepository: openshift/lightspeed-operator
Length of output: 13740
🌐 Web query:
RHOKP image httpd.conf Listen 8080 8443 ssl.conf _default_ exact directives
💡 Result:
In Apache HTTP Server configurations, the Listen directive tells the server to accept incoming connections on specific ports [1]. When running services on non-standard ports, such as 8080 for HTTP and 8443 for HTTPS, the following directives are typically used [1][2]: Listen 8080 Listen 8443 Regarding the default directive, it is used within a VirtualHost block to define a default configuration for requests that do not match any other specific ServerName or ServerAlias directive [3][4]. A common configuration pattern for SSL in ssl.conf is: # SSL and VirtualHost settings here In the context of Red Hat OpenShift, these ports (8080 and 8443) are frequently used because they are non-privileged, allowing containers to bind to them without requiring root privileges [5][6]. While "RHOKP" is not a standard acronym for a core Red Hat product (Red Hat OpenShift is commonly abbreviated as OCP), the use of these specific ports is a standard best practice for deploying Apache httpd within OpenShift containers [5][7]. Configuration files are generally managed via /etc/httpd/conf/httpd.conf for core settings and /etc/httpd/conf.d/ssl.conf for SSL-specific virtual host configurations [8][9][10].
Citations:
- 1: https://httpd.apache.org/docs/current/mod/mpm%5Fcommon.html
- 2: https://developers.redhat.com/blog/2018/03/28/intercede-rapid-red-hat-mobile-openshift
- 3: https://github.com/apache/httpd/blob/trunk/docs/conf/extra/httpd-ssl.conf.in
- 4: https://community.letsencrypt.org/t/ssl-conf-virtualhost-default-443-collides-with-vhosts/52067
- 5: https://gist.github.com/dmc5179/406712bfc4ec2164bfe3257223bef985
- 6: https://docs.redhat.com/en/documentation/openshift_container_platform/2/html/user_guide/sect-binding_applications_to_ports
- 7: https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html/hosted_control_planes/networking-for-hosted-control-planes
- 8: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/9/html/deploying_web_servers_and_reverse_proxies/setting-apache-http-server_deploying-web-servers-and-reverse-proxies
- 9: https://access.redhat.com/articles/3652701
- 10: https://access.redhat.com/articles/1462323
Anchor the sed replacements to the full directive. The current ports are 8080/8443, but these patterns still match numeric prefixes; add an end-of-line boundary ($) so a future Listen 80800 or _default_:84430 line can’t be rewritten accidentally.
🤖 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 `@internal/controller/appserver/rhokp.go` around lines 15 - 25, The command
built in the rhokp helper is matching numeric prefixes too broadly in its sed
replacements. Update the formatting in the function that returns the shell
command so the replacements for the Listen and _default_ directives are anchored
to the full line by adding an end-of-line boundary, preventing accidental
rewrites of values like 80800 or 84430. Use the existing rhokp command
construction symbols and the related utils port/conf path constants to locate
the affected replacements.
| func deploymentContainerCount(base int) int { | ||
| if !cr.Spec.OLSConfig.ByokRAGOnly { | ||
| return base + 1 | ||
| } | ||
| return base | ||
| } | ||
|
|
||
| func expectedAppServerEnv() []corev1.EnvVar { | ||
| env := []corev1.EnvVar{ | ||
| { | ||
| Name: "OLS_CONFIG_FILE", | ||
| Value: filepath.Join(utils.OLSConfigMountRoot, utils.OLSConfigFilename), | ||
| }, | ||
| } | ||
| if !cr.Spec.OLSConfig.ByokRAGOnly { | ||
| env = append(env, corev1.EnvVar{ | ||
| Name: utils.OCPClusterVersionEnvVar, | ||
| Value: testReconcilerInstance.GetOpenShiftMajor() + "." + testReconcilerInstance.GetOpenshiftMinor(), | ||
| }) | ||
| } | ||
| return env | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Helpers close over the wrong cr — variable shadowing bug.
deploymentContainerCount and expectedAppServerEnv reference the package-level cr declared in this file (set once in BeforeSuite and updated only there). However, deployment_test.go's outer Describe("App server deployment generation", ...) declares its own var cr *olsv1alpha1.OLSConfig (local to that closure), which shadows the package-level cr for every BeforeEach/It inside it. Calls like deploymentContainerCount(1) and expectedAppServerEnv() inside those It blocks therefore evaluate ByokRAGOnly against the suite-level cr, not the per-test cr that the test actually mutates.
This currently passes only because the package-level cr's ByokRAGOnly stays false (its zero value) for the whole suite, and none of the existing tests that call these helpers exercise ByokRAGOnly=true — so it's a latent footgun rather than an active failure. Any future test that flips the local cr.Spec.OLSConfig.ByokRAGOnly and then relies on these helpers for container-count/env expectations will silently get wrong expected values.
Pass cr explicitly instead of relying on the ambient package variable.
🐛 Suggested fix
-func deploymentContainerCount(base int) int {
+func deploymentContainerCount(cr *olsv1alpha1.OLSConfig, base int) int {
if !cr.Spec.OLSConfig.ByokRAGOnly {
return base + 1
}
return base
}
-func expectedAppServerEnv() []corev1.EnvVar {
+func expectedAppServerEnv(cr *olsv1alpha1.OLSConfig) []corev1.EnvVar {
env := []corev1.EnvVar{
{
Name: "OLS_CONFIG_FILE",
Value: filepath.Join(utils.OLSConfigMountRoot, utils.OLSConfigFilename),
},
}
if !cr.Spec.OLSConfig.ByokRAGOnly {
env = append(env, corev1.EnvVar{
Name: utils.OCPClusterVersionEnvVar,
Value: testReconcilerInstance.GetOpenShiftMajor() + "." + testReconcilerInstance.GetOpenshiftMinor(),
})
}
return env
}This requires updating all call sites in deployment_test.go to pass the test's local cr (e.g. deploymentContainerCount(cr, 1), expectedAppServerEnv(cr)).
🤖 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 `@internal/controller/appserver/suite_test.go` around lines 196 - 217, The
helper functions deploymentContainerCount and expectedAppServerEnv are closing
over the package-level cr instead of the per-test cr used inside the App server
deployment tests. Update these helpers to accept the relevant OLSConfig/CR as an
explicit argument, and change all call sites in deployment_test.go to pass the
local cr from each BeforeEach/It block so ByokRAGOnly is evaluated against the
correct test state.
|
/override "ci/prow/bundle-e2e-4-21" |
|
@blublinsky: Overrode contexts on behalf of blublinsky: ci/prow/bundle-e2e-4-21 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 kubernetes-sigs/prow repository. |
vimalk78
left a comment
vimalk78
left a comment
There was a problem hiding this comment.
AI Review — OLS-3130: Operator-managed OKP / Solr hybrid RAG
Score: 97/100 — PASS
Adherence
| # | Criterion | Status |
|---|---|---|
| 1 | Create additional fields in the CR to configure both OKP and BYOK tools | PASS |
| 2 | Install OKP image when RAG is configured | PASS |
Code Quality
One should-fix documentation inconsistency:
- Spec/code mismatch on
max_results:.ai/spec/how/config-generation.mdshowsmax_results: 10(unchanged context line from pre-existing spec) butSolrHybridMaxResultsDefault = 5in code. The spec was partially updated (port changed to 9080) butmax_resultswasn't updated to match the new constant.
Notes
- Port layout is clean: RHOKP 9080/9443, MCP 8080, app server 8443 — no conflicts
- Startup probe budget (20s + 34×10s = 6 min) is reasonable for Solr cold start
- RHOKP security context correctly maintains restricted PSS (RunAsNonRoot, no privilege escalation, drop ALL) with only ReadOnlyRootFilesystem relaxed for Solr data
appServerEnv()append is safe —GetProxyEnvVars()returns a fresh slice each call- Good catch fixing pre-existing
McpServerImagetest bug (OLSAppServerImageDefault→OpenShiftMCPServerImageDefault) - Test coverage is thorough: sidecar presence/absence, resource overrides, solr_hybrid config, byokRAGOnly, startup script port remapping
- Bundle regeneration explicitly out of scope per PR description — fine for dev, required before release
🤖 Generated with Claude Code
|
/lgtm |
|
/override "ci/prow/bundle-e2e-4-21" |
|
@blublinsky: Overrode contexts on behalf of blublinsky: ci/prow/bundle-e2e-4-21 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 kubernetes-sigs/prow repository. |
|
@blublinsky: 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: blublinsky 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 |
openshift-ci
Bot
added
the
approved
4 participants
Description
Summary
Operator-only PR for OLS-3130: enable OKP (Offline Knowledge Portal) / Solr hybrid RAG for OpenShift Lightspeed by default.
When
spec.ols.byokRAGOnlyis false (default), the operator:solr_hybridinto the generatedolsconfig.yamlOCP_CLUSTER_VERSIONon the app-server container so the service can serve documentation for the cluster’s OpenShift versionSet
spec.ols.byokRAGOnly: trueto disable OKP (no RHOKP sidecar, nosolr_hybrid, no built-in OpenShift documentation retrieval).OKP is operator-managed — there is no
spec.ols.solrHybrid(or similar) CR field.Service dependency: align with lightspeed-service#2992 / OLS-3313 (OCP product docs via Solr hybrid only; BYOK FAISS via
spec.ols.rag).Out of scope (follow-up PRs)
make bundle) — required before customer releaserelated_images.jsonand CSV--rhokp-imagearg (use--rhokp-imagefor local/make deployuntil then)CR / API
OLSSpec(godoc + CRD base comments only; no new CR fields)byokRAGOnlyand BYOK-focused wording onspec.ols.rag/RAGSpecconfig/crd/bases/); bundle manifests not regenerated in this PRApp server deployment
rhokp) when!byokRAGOnlyhttp://localhost:8080/mcpin OLS config)Listendirectives inhttpd.conf/ssl.confbeforemelstart (ssl.confmust remain present for TLS cert setup)/solr/portal-rag/admin/pingon port 9080; startup probe tuned for cold Solr load (~6 min budget)--rhokp-image(default:registry.redhat.io/offline-knowledge-portal/rhokp-rhel9:latestin code; not in bundle yet)OLS config generation
solr_hybridwith operator defaults (solr_http_base: http://localhost:9080, hybrid retrieval tuning)reference_content.indexes: BYOK only fromspec.ols.rag(no built-in OCP FAISS index; nobyok_index)solr_hybrid/ RHOKP onlyhttp://localhost:8080/mcpLocal dev
make dev-setupappliesconfig/dev/metrics-reader-token.yamlto stop metrics-reader token reconcile loops undermake runType of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Unit tests
make testManual / cluster (recommended before merge)
--rhokp-imagepointing at a known-good RHOKP image.OLSConfigwith default settings (byokRAGOnlyunset/false).rhokpsidecar and MCP sidecar; no port bind conflicts.olsconfigConfigMap contains:solr_hybrid.solr_http_base: http://localhost:9080mcp_serversentryurl: http://localhost:8080/mcpreference_content.indexes(BYOK entries only whenspec.ols.ragis set)curl http://localhost:9080/solr/portal-rag/admin/ping→ 200 after RHOKP startup.spec.ols.byokRAGOnly: true→ RHOKP sidecar removed, nosolr_hybridin config.Container validation (done locally)
hybrid-searchreturn 200.Summary by CodeRabbit
New Features
byokRAGOnly.Bug Fixes
byokRAGOnlyis true.Documentation
Chores