NETOBSERV-2841: Refactor bundling for vendoring#2822
Open
jotak wants to merge 7 commits into
Open
Conversation
Collaborator
|
@jotak: This pull request references NETOBSERV-2554 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 epic to target either version "5.0." or "openshift-5.0.", but it targets "netobserv-2.0" instead. 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. |
📝 WalkthroughWalkthroughConfiguration, controllers, build targets, and generated manifests switch from downstream booleans and console-plugin images to vendor-aware web-console image wiring. Bundle inputs and Helm/OpenShift outputs are regenerated for the new bundle layout and related images. ChangesVendor-aware web-console and bundle regeneration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Member
Author
|
/hold |
jpinsonneau
previously approved these changes
Jun 25, 2026
jpinsonneau
left a comment
jpinsonneau
left a comment
Member
There was a problem hiding this comment.
Looks good in terms of code 👌
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Member
Author
|
/unhold |
jotak
added
the
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm/templates/deployment.yaml (1)
34-41: 🎯 Functional Correctness | 🟡 MinorAdd
RELATED_IMAGE_EBPF_BYTECODEto the Helm deployment environment.The operator expects
RELATED_IMAGE_EBPF_BYTECODEwith a code-level fallback. To align with the guideline "Never hardcode container image references" and allow overrides consistent withEBPF_AGENTand others, pass this variable in the Helm template.Suggestion
- name: RELATED_IMAGE_EBPF_BYTECODE value: '{{ .Values.ebpfAgent.bytecodeImage }}'🤖 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 `@helm/templates/deployment.yaml` around lines 34 - 41, The Helm deployment environment is missing RELATED_IMAGE_EBPF_BYTECODE, which the operator expects with a configurable fallback. Update the deployment template alongside the other RELATED_IMAGE_* entries to pass this variable from values, using the ebpfAgent-related setting so it can be overridden consistently with RELATED_IMAGE_EBPF_AGENT and the other image env vars.
🧹 Nitpick comments (5)
hack/helm-update.sh (1)
36-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuote the glob variable.
Per static analysis (SC2046/SC2086),
$fand$(basename $f)are unquoted, so a path with spaces/globs would word-split. Cheap to harden.♻️ Proposed fix
for f in bundles/k8s/manifests/*_rbac.authorization.k8s.io_v1_clusterrole.yaml; do cp "$f" helm/templates/ - sed -i '1s/^/# Auto-generated from helm-update.sh\n/' helm/templates/$(basename $f) + sed -i '1s/^/# Auto-generated from helm-update.sh\n/' "helm/templates/$(basename "$f")" done🤖 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 `@hack/helm-update.sh` around lines 36 - 39, The loop in helm-update.sh uses unquoted shell expansions, which can word-split or glob-expand unexpectedly. Update the copy and basename usage in the for-loop so the iterated path variable and the generated destination filename are properly quoted, and keep the change localized around the bundle-to-helm template copy logic.Source: Linters/SAST tools
Makefile (1)
434-441: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
$(YQ)consistently.Line 436 invokes
$(YQ)but line 437 calls bareyq. If$(YQ)resolves to a pinned/downloaded binary, the bare call may pick up a different (or missing)yq, producing inconsistent output between the two filters in the same pipe.♻️ Proposed fix
($(KUSTOMIZE) build config/csv \ | $(YQ) '.metadata.annotations.containerImage = "$(IMAGE)"' \ - | yq '.spec.description = load_str("$(BUNDLE_CONFIG)/description.md")' \ + | $(YQ) '.spec.description = load_str("$(BUNDLE_CONFIG)/description.md")' \ ); \🤖 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 `@Makefile` around lines 434 - 441, The bundle generation pipeline is mixing `$(YQ)` with a bare `yq`, which can make the second filter use a different binary or fail entirely. Update the `generate bundle` recipe so both YAML transformations in the same pipe invoke the same `$(YQ)` variable, keeping behavior consistent with the existing `KUSTOMIZE` and `OPSDK` usage.config/k8s/olm/description.md (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGrammar nit: "consisting in" → "consisting of".
This line feeds the generated CSV
.spec.description, so it's user-facing in OLM.📝 Suggested wording
-The NetObserv Operator is a Kubernetes operator for network observability. It deploys a monitoring pipeline consisting in: +The NetObserv Operator is a Kubernetes operator for network observability. It deploys a monitoring pipeline consisting of:🤖 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 `@config/k8s/olm/description.md` at line 1, The operator description text has a grammar issue in the opening sentence. Update the wording in the description content used for the OLM CSV .spec.description by changing the phrase in the sentence handled by the markdown source from “consisting in” to “consisting of”; keep the rest of the NetObserv Operator description unchanged.config/openshift/common/patch.yaml (1)
28-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or explain the commented-out CRD block.
Lines 28-40 are commented-out CRD
inject-cabundlepatches. If they're a deliberate placeholder, add a short note why; otherwise drop them to avoid leaving dead config behind.🤖 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 `@config/openshift/common/patch.yaml` around lines 28 - 40, The commented-out CRD inject-cabundle block in the OpenShift patch should either be removed or documented with a brief explanation if it is intentionally kept as a placeholder. Update the patch content around the CRD entries for flowcollectors.flows.netobserv.io and flowmetrics.flows.netobserv.io so the intent is clear and there is no dead configuration left behind.config/k8s/olm/kustomization.yaml (1)
1-7: 📐 Maintainability & Code Quality | 🔵 TrivialConsolidate Kustomization
basesintoresourcesFile config/k8s/olm/kustomization.yaml contains both
resourcesand the deprecatedbasesfield. Move thebasesentries (../../crd,../../rbac,../../manager,../../webhook,../../certmanager) into theresourceslist.resources: - ../../csv - ../../crd - ../../rbac - ../../manager - ../../webhook - ../../certmanagerRun
make update-bundleafter the change.🤖 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 `@config/k8s/olm/kustomization.yaml` around lines 1 - 7, Consolidate the Kustomization configuration by moving all deprecated bases entries into the existing resources list in kustomization.yaml. Update the Kustomization manifest so the resources field includes ../../csv, ../../crd, ../../rbac, ../../manager, ../../webhook, and ../../certmanager, and remove any separate bases usage. After adjusting the Kustomization entry, run make update-bundle to regenerate the bundle manifests.
🤖 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 @.mk/development.mk:
- Around line 184-189: The set-vendor target is using different namespaces for
the same deployment, which can make the update and rollout checks target
different resources. Update the kubectl set env call in set-vendor to use
$(OPERATOR_NS) consistently, matching the rollout status and wait commands, so
all deployment interactions reference netobserv-controller-manager in the same
namespace.
In `@bundles/k8s/manifests/netobserv-operator.clusterserviceversion.yaml`:
- Around line 1007-1008: The RELATED_IMAGE_EBPF_BYTECODE entry is still using
the mutable main tag, unlike the other pinned image references in the CSV
manifest. Update the value in the netobserv-operator.clusterserviceversion
manifest so it matches the versioned ebpf-bytecode image, using the
RELATED_IMAGE_EBPF_BYTECODE symbol to locate it, and replace main with
v1.11.5-community.
In
`@bundles/openshift/manifests/netobserv-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml`:
- Around line 7-12: The ClusterRole rule for metrics.k8s.io currently grants
create on pods instead of read access; update the RBAC entry in
netobserv-metrics-reader to use the read verbs get, list, and watch so the
metrics API can be queried properly. Locate the rule by its apiGroups
metrics.k8s.io and resources pods fields, and replace the existing verb set
accordingly.
In `@bundles/openshift/manifests/netobserv-operator.clusterserviceversion.yaml`:
- Around line 356-361: The x-descriptor for consolePlugin.imagePullPolicy has a
stray trailing double-quote that breaks the URN and prevents the console widget
from rendering. Fix the descriptor in the source CSV template (the
netobserv-operator.clusterserviceversion YAML under config/csv/bases) by
removing the extra quote from the imagePullPolicy x-descriptor, then regenerate
the OpenShift bundle so the generated manifest matches the corrected descriptor
format.
In `@config/csv/bases/transformed-csv.yaml`:
- Around line 274-279: The consolePlugin.imagePullPolicy x-descriptor has a
stray trailing quote, so remove the extra quote from the urn:alm descriptor
entry in the transformed-csv.yaml block for consolePlugin.imagePullPolicy. Keep
the descriptor consistent with the other imagePullPolicy entries elsewhere in
the file so OLM can recognize the widget correctly.
In `@config/openshift/common/monitor.yaml`:
- Line 18: The `serverName` value in `monitor.yaml` is hardcoded, so the
`Makefile` namespace substitution never applies. Update the OpenShift
configuration by moving this field into a Kustomize patch under
`config/openshift` and make it derive the namespace dynamically, so `serverName`
stays aligned with the deployment namespace for both OLM and manual installs.
Use the existing `monitor.yaml`/OpenShift kustomization flow to locate and
replace the hardcoded `serverName` entry.
In `@internal/controller/flp/flp_common_objects.go`:
- Around line 340-342: The ServiceMonitor TLS setup in serviceMonitor should
treat VendorOpenShiftDownstream the same as VendorOpenShift. Update the call
path around helper.GetServiceMonitorTLSConfig so the OpenShift TLS branch is
selected for both vendor values, likely by adjusting the vendor check in
GetServiceMonitorTLSConfig or normalizing the vendor before calling it. Use the
serviceMonitor function and GetServiceMonitorTLSConfig symbols to locate the
change.
In `@internal/pkg/helper/monitoring.go`:
- Around line 32-55: GetServiceMonitorTLSConfig currently treats only
constants.VendorOpenShift as the OpenShift case, so downstream OpenShift falls
into the CAFile branch instead of the Secret-based TLS config. Update the vendor
check in GetServiceMonitorTLSConfig to include
constants.VendorOpenShiftDownstream as well, matching the handling already used
in config.go and monitoring_objects.go, so both OpenShift variants use the same
service-ca configuration.
In `@internal/pkg/manager/config.go`:
- Around line 57-88: `Config.Validate` only checks `WebConsoleImage`, but
`ResolveWebConsoleImage` can return `WebConsolePF5Image` or `WebConsolePF4Image`
for OpenShift vendors. Update the `Validate` method to require the PF4/PF5 image
fields as well, or otherwise gate validation by the same vendor/version logic
used in `ResolveWebConsoleImage`, so empty variant images are caught before
reconciliation.
---
Outside diff comments:
In `@helm/templates/deployment.yaml`:
- Around line 34-41: The Helm deployment environment is missing
RELATED_IMAGE_EBPF_BYTECODE, which the operator expects with a configurable
fallback. Update the deployment template alongside the other RELATED_IMAGE_*
entries to pass this variable from values, using the ebpfAgent-related setting
so it can be overridden consistently with RELATED_IMAGE_EBPF_AGENT and the other
image env vars.
---
Nitpick comments:
In `@config/k8s/olm/description.md`:
- Line 1: The operator description text has a grammar issue in the opening
sentence. Update the wording in the description content used for the OLM CSV
.spec.description by changing the phrase in the sentence handled by the markdown
source from “consisting in” to “consisting of”; keep the rest of the NetObserv
Operator description unchanged.
In `@config/k8s/olm/kustomization.yaml`:
- Around line 1-7: Consolidate the Kustomization configuration by moving all
deprecated bases entries into the existing resources list in kustomization.yaml.
Update the Kustomization manifest so the resources field includes ../../csv,
../../crd, ../../rbac, ../../manager, ../../webhook, and ../../certmanager, and
remove any separate bases usage. After adjusting the Kustomization entry, run
make update-bundle to regenerate the bundle manifests.
In `@config/openshift/common/patch.yaml`:
- Around line 28-40: The commented-out CRD inject-cabundle block in the
OpenShift patch should either be removed or documented with a brief explanation
if it is intentionally kept as a placeholder. Update the patch content around
the CRD entries for flowcollectors.flows.netobserv.io and
flowmetrics.flows.netobserv.io so the intent is clear and there is no dead
configuration left behind.
In `@hack/helm-update.sh`:
- Around line 36-39: The loop in helm-update.sh uses unquoted shell expansions,
which can word-split or glob-expand unexpectedly. Update the copy and basename
usage in the for-loop so the iterated path variable and the generated
destination filename are properly quoted, and keep the change localized around
the bundle-to-helm template copy logic.
In `@Makefile`:
- Around line 434-441: The bundle generation pipeline is mixing `$(YQ)` with a
bare `yq`, which can make the second filter use a different binary or fail
entirely. Update the `generate bundle` recipe so both YAML transformations in
the same pipe invoke the same `$(YQ)` variable, keeping behavior consistent with
the existing `KUSTOMIZE` and `OPSDK` usage.
🪄 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: Pro
Run ID: a671dbe0-333f-4885-85cd-427622fd05d9
📒 Files selected for processing (119)
.mk/development.mkAGENTS.mdDEVELOPMENT.mdMakefilebundle.Dockerfilebundles/k8s/manifests/flows.netobserv.io_flowcollectors.yamlbundles/k8s/manifests/flows.netobserv.io_flowcollectorslices.yamlbundles/k8s/manifests/flows.netobserv.io_flowmetrics.yamlbundles/k8s/manifests/netobserv-config-watcher_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-expose-metrics_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-flowcollector-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-hostnetwork_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-informers_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-loki-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-loki-writer_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-manager-config_v1_configmap.yamlbundles/k8s/manifests/netobserv-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-metrics-service_v1_service.yamlbundles/k8s/manifests/netobserv-operator.clusterserviceversion.yamlbundles/k8s/manifests/netobserv-token-review_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-webhook-service_v1_service.yamlbundles/k8s/metadata/annotations.yamlbundles/openshift/manifests/flows.netobserv.io_flowcollectors.yamlbundles/openshift/manifests/flows.netobserv.io_flowcollectorslices.yamlbundles/openshift/manifests/flows.netobserv.io_flowmetrics.yamlbundles/openshift/manifests/netobserv-config-watcher_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-expose-metrics_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-flowcollector-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-hostnetwork_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-informers_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-loki-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-loki-writer_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-manager-config_v1_configmap.yamlbundles/openshift/manifests/netobserv-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yamlbundles/openshift/manifests/netobserv-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-metrics-service_v1_service.yamlbundles/openshift/manifests/netobserv-operator-prometheus_rbac.authorization.k8s.io_v1_role.yamlbundles/openshift/manifests/netobserv-operator-prometheus_rbac.authorization.k8s.io_v1_rolebinding.yamlbundles/openshift/manifests/netobserv-operator.clusterserviceversion.yamlbundles/openshift/manifests/netobserv-token-review_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-webhook-service_v1_service.yamlbundles/openshift/metadata/annotations.yamlbundles/openshift/tests/scorecard/config.yamlconfig/csv/bases/netobserv-operator.clusterserviceversion.yamlconfig/csv/bases/transformed-csv.yamlconfig/csv/kustomization.yamlconfig/k8s-olm/kustomization.yamlconfig/k8s/olm/cainjection_in_flowcollectors.yamlconfig/k8s/olm/cainjection_in_flowmetrics.yamlconfig/k8s/olm/description.mdconfig/k8s/olm/kustomization.yamlconfig/k8s/olm/manager_webhook_patch.yamlconfig/k8s/olm/webhookcainjection_patch.yamlconfig/kind/kind.config.yamlconfig/manager/kustomization.yamlconfig/manager/manager.yamlconfig/manager/metrics_service.yamlconfig/manager/namespace.yamlconfig/openshift-olm/default/patch.yamlconfig/openshift-olm/kustomization.yamlconfig/openshift/common/kustomization.yamlconfig/openshift/common/manager-patch.yamlconfig/openshift/common/monitor.yamlconfig/openshift/common/patch.yamlconfig/openshift/kustomization.yamlconfig/openshift/monitor.yamlconfig/openshift/monitoring-patch.yamlconfig/openshift/olm/description.mdconfig/openshift/olm/kustomization.yamlconfig/openshift/olm/tls-servername-patch.yamlconfig/openshift/patch.yamlconfig/webhook/service.yamlhack/crd2csvSpecDesc.shhack/helm-update.shhack/nudging/OWNERShack/nudging/container_digest.shhack/patch_csv.pyhack/update-build.shhelm/crds/flows.netobserv.io_flowcollectors.yamlhelm/crds/flows.netobserv.io_flowcollectorslices.yamlhelm/crds/flows.netobserv.io_flowmetrics.yamlhelm/templates/certificates.yamlhelm/templates/clusterrole.yamlhelm/templates/clusterrolebinding.yamlhelm/templates/deployment.yamlhelm/templates/netobserv-config-watcher_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-expose-metrics_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-flowcollector-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-hostnetwork_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-informers_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-loki-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-loki-writer_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-manager-config_v1_configmap.yamlhelm/templates/netobserv-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-metrics-service_v1_service.yamlhelm/templates/netobserv-token-review_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-webhook-service_v1_service.yamlhelm/templates/role.yamlhelm/templates/rolebinding.yamlhelm/templates/serviceaccount.yamlinternal/controller/constants/constants.gointernal/controller/ebpf/agent_metrics.gointernal/controller/ebpf/internal/permissions/permissions.gointernal/controller/flowcollector_controller.gointernal/controller/flp/flp_common_objects.gointernal/controller/flp/flp_controller.gointernal/controller/flp/flp_monolith_objects.gointernal/controller/flp/flp_transfo_objects.gointernal/controller/monitoring/monitoring_controller.gointernal/controller/monitoring/monitoring_objects.gointernal/controller/networkpolicy/np_objects.gointernal/controller/networkpolicy/np_test.gointernal/controller/reconcilers/common.gointernal/controller/static/static_controller.gointernal/pkg/helper/monitoring.gointernal/pkg/manager/config.gointernal/pkg/manager/config_test.gointernal/pkg/test/envtest.gomain.go
💤 Files with no reviewable changes (15)
- config/openshift-olm/kustomization.yaml
- config/openshift/monitor.yaml
- config/openshift/patch.yaml
- config/openshift/monitoring-patch.yaml
- config/openshift/common/kustomization.yaml
- config/k8s-olm/kustomization.yaml
- config/webhook/service.yaml
- hack/patch_csv.py
- config/manager/metrics_service.yaml
- hack/nudging/container_digest.sh
- config/openshift-olm/default/patch.yaml
- bundles/openshift/manifests/netobserv-webhook-service_v1_service.yaml
- config/kind/kind.config.yaml
- hack/update-build.sh
- hack/nudging/OWNERS
Comment on lines
32
to
55
| func GetServiceMonitorTLSConfig(tls *flowslatest.ServerTLS, serverName string, vendor constants.Vendor) (monitoringv1.Scheme, *monitoringv1.TLSConfig) { | ||
| if tls.Type == flowslatest.TLSAuto { | ||
| if isDownstream { | ||
| if vendor == constants.VendorOpenShift { | ||
| // Upstream prometheus disallows CAFile | ||
| return "https", &monitoringv1.TLSConfig{ | ||
| SafeTLSConfig: monitoringv1.SafeTLSConfig{ | ||
| ServerName: ptr.To(serverName), | ||
| CA: monitoringv1.SecretOrConfigMap{ | ||
| ConfigMap: &corev1.ConfigMapKeySelector{ | ||
| Key: "service-ca.crt", | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: "openshift-service-ca.crt", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| CAFile: "/etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt", | ||
| } | ||
| } | ||
| // Upstream prometheus disallows CAFile | ||
| return "https", &monitoringv1.TLSConfig{ | ||
| SafeTLSConfig: monitoringv1.SafeTLSConfig{ | ||
| ServerName: ptr.To(serverName), | ||
| CA: monitoringv1.SecretOrConfigMap{ | ||
| ConfigMap: &corev1.ConfigMapKeySelector{ | ||
| Key: "service-ca.crt", | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: "openshift-service-ca.crt", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| CAFile: "/etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt", | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Enumerate Vendor constants and any OpenShift helper, plus all comparison sites.
fd -t f 'constants.go' internal/controller/constants -x cat -n {}
echo '--- Vendor comparisons ---'
rg -nP 'Vendor(OpenShift\w*)?\b' internal/ -g '!*_test.go' -C1
echo '--- IsOpenShift-style helpers on Vendor ---'
rg -nP 'func .*Vendor.*\) (Is|IsOpenShift)' internal/Repository: netobserv/netobserv-operator
Length of output: 10953
Incorrect vendor check excludes downstream OpenShift TLS configuration
GetServiceMonitorTLSConfig at internal/pkg/helper/monitoring.go:L34 checks only vendor == constants.VendorOpenShift. Downstream deployments use constants.VendorOpenShiftDownstream, causing them to fall through to the file-based CAFile path which likely does not contain the required CA or behave differently than the Secret-based approach used for upstream.
Other critical paths consistently handle both variants (e.g., config.go:L44, monitoring_objects.go:L28). Update the check to include VendorOpenShiftDownstream to ensure downstream ServiceMonitors use the correct Secret reference:
if vendor == constants.VendorOpenShift || vendor == constants.VendorOpenShiftDownstream {🤖 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/pkg/helper/monitoring.go` around lines 32 - 55,
GetServiceMonitorTLSConfig currently treats only constants.VendorOpenShift as
the OpenShift case, so downstream OpenShift falls into the CAFile branch instead
of the Secret-based TLS config. Update the vendor check in
GetServiceMonitorTLSConfig to include constants.VendorOpenShiftDownstream as
well, matching the handling already used in config.go and monitoring_objects.go,
so both OpenShift variants use the same service-ca configuration.
github-actions
Bot
removed
the
ok-to-test
jotak
added
the
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bundles/k8s/manifests/netobserv-operator.clusterserviceversion.yaml (1)
1004-1020: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore the eBPF bytecode related image wiring.
EBPFByteCodeImagestill exists in the manager config contract, but this CSV no longer providesRELATED_IMAGE_EBPF_BYTECODEor a matchingrelatedImagesentry. That breaks OLM mirroring/reproducibility for the bytecode image and can leave the manager with an empty/default image.Proposed fix
- name: RELATED_IMAGE_DEMO_LOKI value: grafana/loki:3.5.0 + - name: RELATED_IMAGE_EBPF_BYTECODE + value: quay.io/netobserv/ebpf-bytecode:v1.11.5-community - name: NAMESPACE valueFrom: fieldRef: fieldPath: metadata.namespace- image: grafana/loki:3.5.0 name: demo-loki + - image: quay.io/netobserv/ebpf-bytecode:v1.11.5-community + name: ebpf-bytecode#!/bin/bash # Verify the manager config field, env var, and bundle related image stay in sync. rg -nP 'EBPFByteCodeImage|RELATED_IMAGE_EBPF_BYTECODE|ebpf-bytecode' internal bundles config helmAlso applies to: 1141-1153
🤖 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 `@bundles/k8s/manifests/netobserv-operator.clusterserviceversion.yaml` around lines 1004 - 1020, Restore the eBPF bytecode image wiring in the CSV so it stays aligned with the manager config contract: add back the RELATED_IMAGE_EBPF_BYTECODE environment variable in the relatedImages/env block and ensure the matching relatedImages entry is present for the bytecode image. Update the bundle manifest sections that define RELATED_IMAGE_EBPF_AGENT and the other RELATED_IMAGE_* values so EBPFByteCodeImage has a corresponding image source and the manager does not fall back to an empty/default image.
🤖 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 `@Makefile`:
- Around line 414-417: The deploy target is mixing OpenShift manifests with
whatever CSV happens to be in $(BUNDLE_OUT), so make deploy can accidentally
apply the non-OpenShift bundle into an OpenShift install. Update the deploy
recipe to explicitly use the OpenShift bundle output path or OpenShift-specific
CSV source, keeping the existing config/openshift apply and
openshift-netobserv-operator namespace creation aligned with the same OpenShift
bundle flow. Use the deploy target and the
$(BUNDLE_OUT)/manifests/netobserv-operator.clusterserviceversion.yaml reference
as the key places to pin this to OpenShift.
---
Duplicate comments:
In `@bundles/k8s/manifests/netobserv-operator.clusterserviceversion.yaml`:
- Around line 1004-1020: Restore the eBPF bytecode image wiring in the CSV so it
stays aligned with the manager config contract: add back the
RELATED_IMAGE_EBPF_BYTECODE environment variable in the relatedImages/env block
and ensure the matching relatedImages entry is present for the bytecode image.
Update the bundle manifest sections that define RELATED_IMAGE_EBPF_AGENT and the
other RELATED_IMAGE_* values so EBPFByteCodeImage has a corresponding image
source and the manager does not fall back to an empty/default image.
🪄 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: Pro
Run ID: c792ea1b-724f-43dc-b693-ab2226deef13
📒 Files selected for processing (9)
.mk/development.mkMakefilebundles/k8s/manifests/netobserv-operator.clusterserviceversion.yamlbundles/openshift/manifests/netobserv-operator.clusterserviceversion.yamlconfig/csv/bases/netobserv-operator.clusterserviceversion.yamlconfig/csv/bases/transformed-csv.yamlconfig/manager/manager.yamlinternal/pkg/helper/monitoring.gomain.go
💤 Files with no reviewable changes (2)
- config/manager/manager.yaml
- internal/pkg/helper/monitoring.go
✅ Files skipped from review due to trivial changes (1)
- config/csv/bases/transformed-csv.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- config/csv/bases/netobserv-operator.clusterserviceversion.yaml
- .mk/development.mk
- main.go
- bundles/openshift/manifests/netobserv-operator.clusterserviceversion.yaml
github-actions
Bot
removed
the
ok-to-test
jotak
added
the
ok-to-test
github-actions
Bot
removed
the
ok-to-test
jotak
added
the
ok-to-test
github-actions
Bot
removed
the
ok-to-test
jotak
added
the
ok-to-test
github-actions
Bot
removed
the
ok-to-test
jotak
added
the
ok-to-test
Replace the "is downstream" flag with a "vendor" parameter, which is more neutral and makes place for more than openshift. Use only env-based arguments instead of env + command line. Hardcode the vendor-based logic for plugin variant detection (makes code simpler; this didn't look like something people would override anyway) Refactor kustomize configs: - Clearer vendoring with "BUNDLE_TARGET=OpenShift" - All possible targets (2 currently) are now created when running "make update-bundle" - Better separation OpenShift vs Vanilla in OLM config: there's now config/k8s[/olm] on one side and config/openshift[/olm] on the other - Remove a few openshift-specific annotations that were still left in the generic configs - Helm chart is now generated from the k8s bundle, not the openshift one - Avoid creating temporary files here and there; more pipes
github-actions
Bot
removed
the
ok-to-test
jotak
added
the
ok-to-test
|
New images: quay.io/netobserv/network-observability-operator:8a3b2b83
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-8a3b2b83
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-8a3b2b83They will expire in two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:8a3b2b83 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-8a3b2b83Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-8a3b2b83
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
github-actions
Bot
removed
the
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/pkg/helper/monitoring.go`:
- Around line 31-37: The TLSAuto handling in GetServiceMonitorTLSConfig is now
hardcoded to a downstream OpenShift CAFile path, which breaks upstream FLP and
eBPF scrapes. Restore the vendor-aware split in GetServiceMonitorTLSConfig by
preserving the upstream SafeTLSConfig.CA branch and only using the OpenShift
CAFile path for downstream cases, or move that vendor-specific logic back to the
callers that build the ServiceMonitor config.
In `@Makefile`:
- Around line 68-78: `BUNDLE_STORED_DATE` is being evaluated too late, so the
`grep` can read the regenerated CSV instead of the pre-existing `createdAt`
value. Update the Makefile assignment in the bundle date restore logic to use
immediate expansion in the `BUNDLE_STORED_DATE` definition, so the value is
captured before any later bundle generation step runs.
- Around line 447-449: The Makefile validation step in the bundle validation
recipe is only checking the captured text for “warning” and can miss real
failures; update the validation flow around the VALIDATION_OUTPUT assignment so
it also stops on a non-zero exit from the opsdk bundle validate command. Use the
existing bundle validation command and the surrounding shell block to capture
the command status explicitly, then fail the recipe whenever validation returns
an error, not just when the output contains warning text.
🪄 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: Pro
Run ID: bf7d8443-c153-4e86-8e65-15cc603c05c7
📒 Files selected for processing (121)
.github/workflows/build_image_pr.yml.mk/development.mkAGENTS.mdDEVELOPMENT.mdMakefilebundle.Dockerfilebundles/k8s/manifests/flows.netobserv.io_flowcollectors.yamlbundles/k8s/manifests/flows.netobserv.io_flowcollectorslices.yamlbundles/k8s/manifests/flows.netobserv.io_flowmetrics.yamlbundles/k8s/manifests/netobserv-config-watcher_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-expose-metrics_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-flowcollector-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-hostnetwork_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-informers_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-loki-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-loki-writer_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-manager-config_v1_configmap.yamlbundles/k8s/manifests/netobserv-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-metrics-service_v1_service.yamlbundles/k8s/manifests/netobserv-operator.clusterserviceversion.yamlbundles/k8s/manifests/netobserv-token-review_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/k8s/manifests/netobserv-webhook-service_v1_service.yamlbundles/k8s/metadata/annotations.yamlbundles/openshift/manifests/flows.netobserv.io_flowcollectors.yamlbundles/openshift/manifests/flows.netobserv.io_flowcollectorslices.yamlbundles/openshift/manifests/flows.netobserv.io_flowmetrics.yamlbundles/openshift/manifests/netobserv-config-watcher_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-expose-metrics_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-flowcollector-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-hostnetwork_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-informers_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-loki-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-loki-writer_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-manager-config_v1_configmap.yamlbundles/openshift/manifests/netobserv-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yamlbundles/openshift/manifests/netobserv-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-metrics-service_v1_service.yamlbundles/openshift/manifests/netobserv-operator-prometheus_rbac.authorization.k8s.io_v1_role.yamlbundles/openshift/manifests/netobserv-operator-prometheus_rbac.authorization.k8s.io_v1_rolebinding.yamlbundles/openshift/manifests/netobserv-operator.clusterserviceversion.yamlbundles/openshift/manifests/netobserv-token-review_rbac.authorization.k8s.io_v1_clusterrole.yamlbundles/openshift/manifests/netobserv-webhook-service_v1_service.yamlbundles/openshift/metadata/annotations.yamlbundles/openshift/tests/scorecard/config.yamlconfig/csv/bases/netobserv-operator.clusterserviceversion.yamlconfig/csv/bases/transformed-csv.yamlconfig/csv/kustomization.yamlconfig/k8s-olm/kustomization.yamlconfig/k8s/olm/cainjection_in_flowcollectors.yamlconfig/k8s/olm/cainjection_in_flowmetrics.yamlconfig/k8s/olm/description.mdconfig/k8s/olm/kustomization.yamlconfig/k8s/olm/manager_webhook_patch.yamlconfig/k8s/olm/webhookcainjection_patch.yamlconfig/kind/kind.config.yamlconfig/manager/kustomization.yamlconfig/manager/manager.yamlconfig/manager/metrics_service.yamlconfig/manager/namespace.yamlconfig/openshift-olm/default/patch.yamlconfig/openshift-olm/kustomization.yamlconfig/openshift/common/kustomization.yamlconfig/openshift/common/manager-patch.yamlconfig/openshift/common/monitor.yamlconfig/openshift/common/patch.yamlconfig/openshift/kustomization.yamlconfig/openshift/monitor.yamlconfig/openshift/monitoring-patch.yamlconfig/openshift/olm/description.mdconfig/openshift/olm/kustomization.yamlconfig/openshift/olm/tls-servername-patch.yamlconfig/openshift/patch.yamlconfig/webhook/service.yamlhack/crd2csvSpecDesc.shhack/helm-update.shhack/nudging/OWNERShack/nudging/container_digest.shhack/patch_csv.pyhack/update-build.shhack/update_fbc.shhelm/crds/flows.netobserv.io_flowcollectors.yamlhelm/crds/flows.netobserv.io_flowcollectorslices.yamlhelm/crds/flows.netobserv.io_flowmetrics.yamlhelm/templates/certificates.yamlhelm/templates/clusterrole.yamlhelm/templates/clusterrolebinding.yamlhelm/templates/deployment.yamlhelm/templates/netobserv-config-watcher_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-expose-metrics_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-flowcollector-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-hostnetwork_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-informers_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-loki-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-loki-writer_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-manager-config_v1_configmap.yamlhelm/templates/netobserv-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-metrics-service_v1_service.yamlhelm/templates/netobserv-token-review_rbac.authorization.k8s.io_v1_clusterrole.yamlhelm/templates/netobserv-webhook-service_v1_service.yamlhelm/templates/role.yamlhelm/templates/rolebinding.yamlhelm/templates/serviceaccount.yamlinternal/controller/constants/constants.gointernal/controller/ebpf/agent_metrics.gointernal/controller/ebpf/internal/permissions/permissions.gointernal/controller/flowcollector_controller.gointernal/controller/flp/flp_common_objects.gointernal/controller/flp/flp_controller.gointernal/controller/flp/flp_monolith_objects.gointernal/controller/flp/flp_transfo_objects.gointernal/controller/monitoring/monitoring_controller.gointernal/controller/monitoring/monitoring_objects.gointernal/controller/networkpolicy/np_objects.gointernal/controller/networkpolicy/np_test.gointernal/controller/reconcilers/common.gointernal/controller/static/static_controller.gointernal/pkg/helper/monitoring.gointernal/pkg/manager/config.gointernal/pkg/manager/config_test.gointernal/pkg/test/envtest.gomain.go
💤 Files with no reviewable changes (17)
- config/openshift/common/kustomization.yaml
- config/openshift/monitoring-patch.yaml
- config/k8s-olm/kustomization.yaml
- config/webhook/service.yaml
- internal/controller/flp/flp_transfo_objects.go
- hack/nudging/OWNERS
- config/manager/metrics_service.yaml
- config/openshift-olm/default/patch.yaml
- config/openshift/patch.yaml
- config/openshift-olm/kustomization.yaml
- config/openshift/monitor.yaml
- config/kind/kind.config.yaml
- bundles/openshift/manifests/netobserv-webhook-service_v1_service.yaml
- hack/nudging/container_digest.sh
- hack/update-build.sh
- hack/patch_csv.py
- internal/controller/flp/flp_monolith_objects.go
✅ Files skipped from review due to trivial changes (39)
- helm/templates/serviceaccount.yaml
- config/manager/namespace.yaml
- helm/templates/clusterrole.yaml
- helm/templates/netobserv-loki-writer_rbac.authorization.k8s.io_v1_clusterrole.yaml
- helm/crds/flows.netobserv.io_flowcollectorslices.yaml
- helm/templates/role.yaml
- helm/templates/netobserv-webhook-service_v1_service.yaml
- bundles/openshift/manifests/netobserv-manager-config_v1_configmap.yaml
- helm/templates/netobserv-metrics-service_v1_service.yaml
- bundles/k8s/manifests/netobserv-webhook-service_v1_service.yaml
- helm/crds/flows.netobserv.io_flowmetrics.yaml
- helm/templates/rolebinding.yaml
- bundle.Dockerfile
- helm/templates/netobserv-token-review_rbac.authorization.k8s.io_v1_clusterrole.yaml
- helm/templates/netobserv-informers_rbac.authorization.k8s.io_v1_clusterrole.yaml
- helm/templates/clusterrolebinding.yaml
- helm/templates/certificates.yaml
- config/openshift/olm/tls-servername-patch.yaml
- bundles/openshift/manifests/netobserv-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml
- helm/templates/netobserv-flowcollector-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
- config/openshift/common/patch.yaml
- helm/templates/netobserv-loki-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml
- helm/templates/netobserv-manager-config_v1_configmap.yaml
- config/openshift/common/manager-patch.yaml
- AGENTS.md
- config/manager/kustomization.yaml
- config/k8s/olm/description.md
- helm/templates/netobserv-config-watcher_rbac.authorization.k8s.io_v1_clusterrole.yaml
- bundles/k8s/manifests/netobserv-metrics-service_v1_service.yaml
- helm/templates/netobserv-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml
- helm/crds/flows.netobserv.io_flowcollectors.yaml
- bundles/openshift/manifests/netobserv-loki-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml
- helm/templates/netobserv-expose-metrics_rbac.authorization.k8s.io_v1_clusterrole.yaml
- helm/templates/netobserv-hostnetwork_rbac.authorization.k8s.io_v1_clusterrole.yaml
- internal/controller/monitoring/monitoring_controller.go
- config/csv/bases/transformed-csv.yaml
- DEVELOPMENT.md
- bundles/openshift/manifests/netobserv-config-watcher_rbac.authorization.k8s.io_v1_clusterrole.yaml
- bundles/openshift/manifests/netobserv-operator-prometheus_rbac.authorization.k8s.io_v1_rolebinding.yaml
🚧 Files skipped from review as they are similar to previous changes (36)
- config/openshift/olm/kustomization.yaml
- bundles/openshift/manifests/netobserv-loki-writer_rbac.authorization.k8s.io_v1_clusterrole.yaml
- internal/controller/flowcollector_controller.go
- bundles/openshift/manifests/netobserv-operator-prometheus_rbac.authorization.k8s.io_v1_role.yaml
- bundles/openshift/manifests/netobserv-expose-metrics_rbac.authorization.k8s.io_v1_clusterrole.yaml
- bundles/openshift/manifests/netobserv-flowcollector-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
- internal/controller/reconcilers/common.go
- hack/update_fbc.sh
- hack/crd2csvSpecDesc.sh
- internal/controller/networkpolicy/np_test.go
- internal/controller/ebpf/agent_metrics.go
- config/csv/kustomization.yaml
- config/openshift/common/monitor.yaml
- config/csv/bases/netobserv-operator.clusterserviceversion.yaml
- bundles/openshift/manifests/flows.netobserv.io_flowcollectorslices.yaml
- config/k8s/olm/kustomization.yaml
- bundles/openshift/manifests/netobserv-token-review_rbac.authorization.k8s.io_v1_clusterrole.yaml
- bundles/openshift/manifests/netobserv-hostnetwork_rbac.authorization.k8s.io_v1_clusterrole.yaml
- internal/controller/static/static_controller.go
- internal/controller/flp/flp_controller.go
- internal/controller/constants/constants.go
- helm/templates/deployment.yaml
- bundles/openshift/manifests/netobserv-informers_rbac.authorization.k8s.io_v1_clusterrole.yaml
- config/openshift/kustomization.yaml
- bundles/openshift/manifests/flows.netobserv.io_flowmetrics.yaml
- internal/controller/monitoring/monitoring_objects.go
- bundles/k8s/metadata/annotations.yaml
- config/manager/manager.yaml
- .mk/development.mk
- internal/pkg/manager/config_test.go
- internal/pkg/test/envtest.go
- internal/controller/ebpf/internal/permissions/permissions.go
- internal/pkg/manager/config.go
- main.go
- bundles/openshift/manifests/netobserv-operator.clusterserviceversion.yaml
- bundles/k8s/manifests/netobserv-operator.clusterserviceversion.yaml
Comment on lines
+31
to
+37
| func GetServiceMonitorTLSConfig(tls *flowslatest.ServerTLS, serverName string) (monitoringv1.Scheme, *monitoringv1.TLSConfig) { | ||
| if tls.Type == flowslatest.TLSAuto { | ||
| if isDownstream { | ||
| return "https", &monitoringv1.TLSConfig{ | ||
| SafeTLSConfig: monitoringv1.SafeTLSConfig{ | ||
| ServerName: ptr.To(serverName), | ||
| }, | ||
| CAFile: "/etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt", | ||
| } | ||
| } | ||
| // Upstream prometheus disallows CAFile | ||
| return "https", &monitoringv1.TLSConfig{ | ||
| SafeTLSConfig: monitoringv1.SafeTLSConfig{ | ||
| ServerName: ptr.To(serverName), | ||
| CA: monitoringv1.SecretOrConfigMap{ | ||
| ConfigMap: &corev1.ConfigMapKeySelector{ | ||
| Key: "service-ca.crt", | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: "openshift-service-ca.crt", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| CAFile: "/etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore vendor-aware TLSAuto handling.
This now emits the downstream OpenShift CAFile path for every TLSAuto ServiceMonitor. The upstream SafeTLSConfig.CA branch is gone, so FLP and eBPF auto-TLS scrapes can break outside downstream OpenShift. Keep the vendor split here, or restore it at the callers, instead of hardcoding a single CA source.
🤖 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/pkg/helper/monitoring.go` around lines 31 - 37, The TLSAuto handling
in GetServiceMonitorTLSConfig is now hardcoded to a downstream OpenShift CAFile
path, which breaks upstream FLP and eBPF scrapes. Restore the vendor-aware split
in GetServiceMonitorTLSConfig by preserving the upstream SafeTLSConfig.CA branch
and only using the OpenShift CAFile path for downstream cases, or move that
vendor-specific logic back to the callers that build the ServiceMonitor config.
Comment on lines
+68
to
+78
| ifeq ("$(BUNDLE_TARGET)", "OpenShift") | ||
| BUNDLE_CONFIG = config/openshift/olm | ||
| BUNDLE_OUT = bundles/openshift | ||
| else | ||
| BUNDLE_CONFIG = config/k8s/olm | ||
| BUNDLE_OUT = bundles/k8s | ||
| endif | ||
|
|
||
| # If we don't want to set bundle date (upon bundle update call), store current date | ||
| ifneq ("$(BUNDLE_SET_DATE)", "true") | ||
| BUNDLE_STORED_DATE = $(shell grep "createdAt:" bundle/manifests/netobserv-operator.clusterserviceversion.yaml | sed -r 's/^.*createdAt:[ ]*(.*)/\1/') | ||
| BUNDLE_STORED_DATE = $(shell grep "createdAt:" $(BUNDLE_OUT)/manifests/netobserv-operator.clusterserviceversion.yaml | sed -r 's/^.*createdAt:[ ]*(.*)/\1/') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Capture createdAt before regenerating the bundle.
BUNDLE_STORED_DATE is recursively expanded, so the grep can run when Line 444 restores the date, after the new CSV has already been generated. Use immediate assignment to preserve the pre-existing value.
Proposed fix
- BUNDLE_STORED_DATE = $(shell grep "createdAt:" $(BUNDLE_OUT)/manifests/netobserv-operator.clusterserviceversion.yaml | sed -r 's/^.*createdAt:[ ]*(.*)/\1/')
+ BUNDLE_STORED_DATE := $(shell grep "createdAt:" $(BUNDLE_OUT)/manifests/netobserv-operator.clusterserviceversion.yaml | sed -r 's/^.*createdAt:[ ]*(.*)/\1/')📝 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.
Suggested change
| ifeq ("$(BUNDLE_TARGET)", "OpenShift") | |
| BUNDLE_CONFIG = config/openshift/olm | |
| BUNDLE_OUT = bundles/openshift | |
| else | |
| BUNDLE_CONFIG = config/k8s/olm | |
| BUNDLE_OUT = bundles/k8s | |
| endif | |
| # If we don't want to set bundle date (upon bundle update call), store current date | |
| ifneq ("$(BUNDLE_SET_DATE)", "true") | |
| BUNDLE_STORED_DATE = $(shell grep "createdAt:" bundle/manifests/netobserv-operator.clusterserviceversion.yaml | sed -r 's/^.*createdAt:[ ]*(.*)/\1/') | |
| BUNDLE_STORED_DATE = $(shell grep "createdAt:" $(BUNDLE_OUT)/manifests/netobserv-operator.clusterserviceversion.yaml | sed -r 's/^.*createdAt:[ ]*(.*)/\1/') | |
| ifeq ("$(BUNDLE_TARGET)", "OpenShift") | |
| BUNDLE_CONFIG = config/openshift/olm | |
| BUNDLE_OUT = bundles/openshift | |
| else | |
| BUNDLE_CONFIG = config/k8s/olm | |
| BUNDLE_OUT = bundles/k8s | |
| endif | |
| # If we don't want to set bundle date (upon bundle update call), store current date | |
| ifneq ("$(BUNDLE_SET_DATE)", "true") | |
| BUNDLE_STORED_DATE := $(shell grep "createdAt:" $(BUNDLE_OUT)/manifests/netobserv-operator.clusterserviceversion.yaml | sed -r 's/^.*createdAt:[ ]*(.*)/\1/') |
🤖 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 `@Makefile` around lines 68 - 78, `BUNDLE_STORED_DATE` is being evaluated too
late, so the `grep` can read the regenerated CSV instead of the pre-existing
`createdAt` value. Update the Makefile assignment in the bundle date restore
logic to use immediate expansion in the `BUNDLE_STORED_DATE` definition, so the
value is captured before any later bundle generation step runs.
Comment on lines
+447
to
449
| VALIDATION_OUTPUT=$$($(OPSDK) bundle validate $(BUNDLE_OUT) --select-optional suite=operatorframework); \ | ||
| echo $${VALIDATION_OUTPUT}; \ | ||
| if [ $$(echo $${VALIDATION_OUTPUT} | grep -i 'warning' | wc -c) -gt 0 ]; then echo "please correct warnings and errors first"; exit -1 ; fi \ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail when bundle validation exits non-zero.
VALIDATION_OUTPUT=$$(...) captures the validation output, but the recipe then continues and only fails if the text contains warning. Validation errors without that word can pass.
Proposed fix
- VALIDATION_OUTPUT=$$($(OPSDK) bundle validate $(BUNDLE_OUT) --select-optional suite=operatorframework); \
- echo $${VALIDATION_OUTPUT}; \
- if [ $$(echo $${VALIDATION_OUTPUT} | grep -i 'warning' | wc -c) -gt 0 ]; then echo "please correct warnings and errors first"; exit -1 ; fi \
+ VALIDATION_OUTPUT=$$($(OPSDK) bundle validate $(BUNDLE_OUT) --select-optional suite=operatorframework 2>&1); \
+ VALIDATION_STATUS=$$?; \
+ echo "$${VALIDATION_OUTPUT}"; \
+ if [ $${VALIDATION_STATUS} -ne 0 ] || echo "$${VALIDATION_OUTPUT}" | grep -iq 'warning'; then echo "please correct warnings and errors first"; exit 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.
Suggested change
| VALIDATION_OUTPUT=$$($(OPSDK) bundle validate $(BUNDLE_OUT) --select-optional suite=operatorframework); \ | |
| echo $${VALIDATION_OUTPUT}; \ | |
| if [ $$(echo $${VALIDATION_OUTPUT} | grep -i 'warning' | wc -c) -gt 0 ]; then echo "please correct warnings and errors first"; exit -1 ; fi \ | |
| VALIDATION_OUTPUT=$$($(OPSDK) bundle validate $(BUNDLE_OUT) --select-optional suite=operatorframework 2>&1); \ | |
| VALIDATION_STATUS=$$?; \ | |
| echo "$${VALIDATION_OUTPUT}"; \ | |
| if [ $${VALIDATION_STATUS} -ne 0 ] || echo "$${VALIDATION_OUTPUT}" | grep -iq 'warning'; then echo "please correct warnings and errors first"; exit 1 ; fi \ |
🤖 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 `@Makefile` around lines 447 - 449, The Makefile validation step in the bundle
validation recipe is only checking the captured text for “warning” and can miss
real failures; update the validation flow around the VALIDATION_OUTPUT
assignment so it also stops on a non-zero exit from the opsdk bundle validate
command. Use the existing bundle validation command and the surrounding shell
block to capture the command status explicitly, then fail the recipe whenever
validation returns an error, not just when the output contains warning text.
memodi
reviewed
Jun 25, 2026
Member
There was a problem hiding this comment.
much of our testing like nightly perf test and daily testing uses upstream bundle v0.0.0-main , could you summarize how that bundle would be impacted with changes here?
at minimum for env vars update, we'd need to update some of our scripts:
for perf tests https://github.com/netobserv/netobserv-perf-tests/blob/main/scripts/netobserv.sh#L105-L133
https://gitlab.cee.redhat.com/netobserv-qe/netobserv-qe-scripts/-/blob/main/patch_netobserv.sh?ref_type=heads
how does metrics publishing now will be controlled in upstream bundle tag with v0.0.0-sha-main?
it seems now with upstream bundle operator bundle will be installed in system ns, true? if so, we'd need to change perf tests metrics collection for this.
Member
Author
Nothing should change, we'll continue to generate a
I see, yes that will have to change, similar to what I did on openshift/release
"will be controlled" => do you mean that you want to turn it on and off? It doesn't have to be always on?
No no, "system" is the default that operator-sdk uses when bootstrapping the project, but it's a placeholder. I've set it in some places to "system" just for consistency (it was confusing to see different names for placeholders). At the end of the day, it should stay the same as before in the bundle. |
Member
always on is definitely good if that's how it's going to be. We should definitely run some tests when this is ready on the newer bundles from this PR. cc @Amoghrd @kapjain-rh |
jotak
added
the
ok-to-test
jotak
changed the title
Collaborator
|
@jotak: This pull request references NETOBSERV-2841 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. |
|
New images: quay.io/netobserv/network-observability-operator:c8096456
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-c8096456
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-c8096456They will expire in two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:c8096456 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-c8096456Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-c8096456
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
|
@jotak: The following test failed, say
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. |
Member
Author
|
@memodi it's fine to be reviewed - I ran some smoke tests with the image above ( |
Member
|
/lgtm (verified metrics publish and install works well) |
|
PR needs rebase. 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. |
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
4 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.
Description
Replace the "is downstream" flag with a "vendor" parameter, which is more neutral and makes place for more than openshift.
Use only env-based arguments instead of env + command line. Hardcode the vendor-based logic for plugin variant detection (makes code simpler; this didn't look like something people would override anyway)
Refactor kustomize configs:
Dependencies
Checklist
Summary by CodeRabbit