feat: update perses and perses operator#1067
Conversation
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates CRD OpenAPI schemas for multiple Perses CRDs (adds conditional CEL validations, minLength constraints, changes status.conditions to map-style lists keyed by Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
/test all |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
13-41:⚠️ Potential issue | 🟠 MajorAlign the replaced dependency versions with the required versions.
Line 39 and Line 41 override the newer versions required on Line 13 and Line 34, so builds still use the older
controller-runtime-commonandcontroller-runtimeversions. Either remove the replacements to actually upgrade, or make therequireversions match the pinned replacements.🔎 Read-only verification
This checks whether the modules are required at one version but replaced with a different version.
#!/bin/bash set -euo pipefail awk ' $1 == "github.com/openshift/controller-runtime-common" { print "require/replace candidate:", $0 } $1 == "sigs.k8s.io/controller-runtime" { print "require/replace candidate:", $0 } ' go.modExpected result after fixing: each module should either have no replacement, or the replacement version should intentionally match the effective pinned version.
🛠️ Possible fixes
If the intent is to complete the upgrade:
replace ( github.com/openshift/api => github.com/openshift/api v0.0.0-20240404200104-96ed2d49b255 - github.com/openshift/controller-runtime-common => github.com/openshift/controller-runtime-common v0.0.0-20260210092218-8eef974290cd github.com/rhobs/observability-operator/pkg/apis => ./pkg/apis - sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.22.5 )If the older pins are intentional, align the
requireblock instead:- github.com/openshift/controller-runtime-common v0.0.0-20260318085703-1812aed6dbd2 + github.com/openshift/controller-runtime-common v0.0.0-20260210092218-8eef974290cd ... - sigs.k8s.io/controller-runtime v0.23.3 + sigs.k8s.io/controller-runtime v0.22.5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 13 - 41, go.mod currently replaces newer required modules with older pins so the effective versions differ; either remove the replace entries for github.com/openshift/controller-runtime-common and sigs.k8s.io/controller-runtime in the replace (...) block to let the require (...) versions take effect, or update the require (...) entries to match the pinned versions in the replace (...) block (i.e., make the require version for github.com/openshift/controller-runtime-common and sigs.k8s.io/controller-runtime equal to the versions listed in replace), and then run go mod tidy to verify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@go.mod`:
- Around line 13-41: go.mod currently replaces newer required modules with older
pins so the effective versions differ; either remove the replace entries for
github.com/openshift/controller-runtime-common and
sigs.k8s.io/controller-runtime in the replace (...) block to let the require
(...) versions take effect, or update the require (...) entries to match the
pinned versions in the replace (...) block (i.e., make the require version for
github.com/openshift/controller-runtime-common and
sigs.k8s.io/controller-runtime equal to the versions listed in replace), and
then run go mod tidy to verify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b89238cd-d9dd-4bd1-b1fa-3eabd27fa760
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
Makefiledeploy/perses/crds/perses.dev_perses.yamldeploy/perses/crds/perses.dev_persesdashboards.yamldeploy/perses/crds/perses.dev_persesdatasources.yamldeploy/perses/crds/perses.dev_persesglobaldatasources.yamldeploy/perses/perses-operator-cluster-role.yamldeploy/perses/perses-operator-deployment.yamlgo.modpkg/controllers/uiplugin/accelerators.gopkg/controllers/uiplugin/apm.gopkg/controllers/uiplugin/monitoring.go
jgbernalp
force-pushed
the
update-perses-components
branch
from
0dd4eca to
be7fe1d
Compare
|
/hold for #1068 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
deploy/perses/crds/perses.dev_persesglobaldatasources.yaml (1)
233-279:⚠️ Potential issue | 🟠 MajorRequire
privateKeyPathforuserCert.The description says
privateKeyPathis required for client certificates, but the schema still acceptsuserCertobjects without it. That allows invalid mTLS configs through admission and can fail later at reconcile/runtime. Please update the source markers and regenerate both deploy and bundle CRDs.Proposed schema fix
privateKeyPath: description: |- privateKeyPath specifies the key name within the secret/configmap or filesystem path (depending on SecretSource.Type) where the private key is stored Required for client certificates (UserCert), optional for CA certificates (CaCert) + minLength: 1 type: string @@ required: - certPath + - privateKeyPath - type🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/perses/crds/perses.dev_persesglobaldatasources.yaml` around lines 233 - 279, The schema for userCert allows missing privateKeyPath though the description says it's required for client certificates; update the userCert object (properties: privateKeyPath and required list) so that privateKeyPath is marked required whenever type indicates a client certificate and/or add an x-kubernetes-validations rule referencing self.type and has(self.privateKeyPath) to enforce presence for secret/configmap/file as appropriate; modify the required array to include "privateKeyPath" for the unconditional case or add the conditional x-kubernetes-validations rule (alongside the existing name/namespace rules) and then regenerate the deploy and bundle CRDs so the changes are propagated.bundle/manifests/perses.dev_persesdatasources.yaml (1)
536-582:⚠️ Potential issue | 🟠 MajorKeep the bundle schema aligned and require
privateKeyPathforuserCert.The bundle CRD has the same gap:
userCert.privateKeyPathis documented as required for client certificates, but admission does not enforce it.Proposed bundle schema fix
privateKeyPath: description: |- privateKeyPath specifies the key name within the secret/configmap or filesystem path (depending on SecretSource.Type) where the private key is stored Required for client certificates (UserCert), optional for CA certificates (CaCert) + minLength: 1 type: string @@ required: - certPath + - privateKeyPath - type🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundle/manifests/perses.dev_persesdatasources.yaml` around lines 536 - 582, The userCert schema currently documents that privateKeyPath is required for client certificates but does not enforce it; update the userCert object schema (the required array for userCert) to include "privateKeyPath" and/or add an x-kubernetes-validations rule that requires has(self.privateKeyPath) when self.type is "secret" or "configmap" or when a client certificate is expected; modify the required list and validation rules under the userCert definition so privateKeyPath is enforced consistently with certPath/type/name/namespace checks.deploy/perses/crds/perses.dev_persesdatasources.yaml (1)
535-581:⚠️ Potential issue | 🟠 MajorRequire
privateKeyPathforuserCert.
userCertis documented as client cert/key material, but the schema still admits auserCertwithout a private key path. That can let invalid mTLS configs pass admission and fail later during reconciliation/runtime.Proposed CRD schema fix
privateKeyPath: description: |- privateKeyPath specifies the key name within the secret/configmap or filesystem path (depending on SecretSource.Type) where the private key is stored Required for client certificates (UserCert), optional for CA certificates (CaCert) + minLength: 1 type: string @@ required: - certPath + - privateKeyPath - type🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/perses/crds/perses.dev_persesdatasources.yaml` around lines 535 - 581, The userCert object currently allows omitting the private key path which breaks mTLS; add 'privateKeyPath' to the userCert.required array so the schema requires the privateKeyPath field, and if you want parity with other fields also add an x-kubernetes-validations rule (under the existing x-kubernetes-validations for userCert) that enforces has(self.privateKeyPath) when the object represents a client certificate (e.g., always for userCert or when self.type == 'secret' || self.type == 'configmap' || self.type == 'file'); update the required list and/or validation rule referencing userCert, privateKeyPath, and x-kubernetes-validations.
♻️ Duplicate comments (1)
bundle/manifests/perses.dev_persesglobaldatasources.yaml (1)
244-290:⚠️ Potential issue | 🟠 MajorMirror the
userCert.privateKeyPathschema fix here too.This bundle CRD has the same generated validation gap:
privateKeyPathis documented as required for client certificates but is not required by the schema. Regenerate the bundle after fixing the CRD source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundle/manifests/perses.dev_persesglobaldatasources.yaml` around lines 244 - 290, The CRD schema for userCert omits privateKeyPath from the required list even though the docs state it is required for client certificates; add "privateKeyPath" to the userCert.required array (alongside "certPath" and "type") so the field is enforced, keep the existing x-kubernetes-validations (name/namespace rules) intact, and then regenerate the bundle so bundle/manifests/perses.dev_persesglobaldatasources.yaml reflects the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bundle/manifests/perses.dev_persesdatasources.yaml`:
- Around line 536-582: The userCert schema currently documents that
privateKeyPath is required for client certificates but does not enforce it;
update the userCert object schema (the required array for userCert) to include
"privateKeyPath" and/or add an x-kubernetes-validations rule that requires
has(self.privateKeyPath) when self.type is "secret" or "configmap" or when a
client certificate is expected; modify the required list and validation rules
under the userCert definition so privateKeyPath is enforced consistently with
certPath/type/name/namespace checks.
In `@deploy/perses/crds/perses.dev_persesdatasources.yaml`:
- Around line 535-581: The userCert object currently allows omitting the private
key path which breaks mTLS; add 'privateKeyPath' to the userCert.required array
so the schema requires the privateKeyPath field, and if you want parity with
other fields also add an x-kubernetes-validations rule (under the existing
x-kubernetes-validations for userCert) that enforces has(self.privateKeyPath)
when the object represents a client certificate (e.g., always for userCert or
when self.type == 'secret' || self.type == 'configmap' || self.type == 'file');
update the required list and/or validation rule referencing userCert,
privateKeyPath, and x-kubernetes-validations.
In `@deploy/perses/crds/perses.dev_persesglobaldatasources.yaml`:
- Around line 233-279: The schema for userCert allows missing privateKeyPath
though the description says it's required for client certificates; update the
userCert object (properties: privateKeyPath and required list) so that
privateKeyPath is marked required whenever type indicates a client certificate
and/or add an x-kubernetes-validations rule referencing self.type and
has(self.privateKeyPath) to enforce presence for secret/configmap/file as
appropriate; modify the required array to include "privateKeyPath" for the
unconditional case or add the conditional x-kubernetes-validations rule
(alongside the existing name/namespace rules) and then regenerate the deploy and
bundle CRDs so the changes are propagated.
---
Duplicate comments:
In `@bundle/manifests/perses.dev_persesglobaldatasources.yaml`:
- Around line 244-290: The CRD schema for userCert omits privateKeyPath from the
required list even though the docs state it is required for client certificates;
add "privateKeyPath" to the userCert.required array (alongside "certPath" and
"type") so the field is enforced, keep the existing x-kubernetes-validations
(name/namespace rules) intact, and then regenerate the bundle so
bundle/manifests/perses.dev_persesglobaldatasources.yaml reflects the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: efed8980-53a5-44f1-b35d-1e75374a5dcb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
Makefilebundle/manifests/observability-operator.clusterserviceversion.yamlbundle/manifests/perses.dev_perses.yamlbundle/manifests/perses.dev_persesdashboards.yamlbundle/manifests/perses.dev_persesdatasources.yamlbundle/manifests/perses.dev_persesglobaldatasources.yamldeploy/perses/crds/perses.dev_perses.yamldeploy/perses/crds/perses.dev_persesdashboards.yamldeploy/perses/crds/perses.dev_persesdatasources.yamldeploy/perses/crds/perses.dev_persesglobaldatasources.yamldeploy/perses/perses-operator-cluster-role.yamldeploy/perses/perses-operator-deployment.yamlgo.modpkg/controllers/uiplugin/accelerators.gopkg/controllers/uiplugin/apm.gopkg/controllers/uiplugin/monitoring.go
✅ Files skipped from review due to trivial changes (2)
- deploy/perses/perses-operator-deployment.yaml
- deploy/perses/perses-operator-cluster-role.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controllers/uiplugin/apm.go
- deploy/perses/crds/perses.dev_persesdashboards.yaml
jgbernalp
force-pushed
the
update-perses-components
branch
from
be7fe1d to
57f75e3
Compare
|
/unhold |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bundle/manifests/observability-operator.clusterserviceversion.yaml`:
- Around line 1175-1178: The perses-operator container args include invalid
flags (--tls-cluster-profile and --tls-configure-operands) which should be
removed or replaced; locate the perses-operator container args block (the image
quay.io/openshift-observability-ui/perses-operator:v0.3.0-go-1.25 and its args
list) and delete these two flags, or replace them with valid perses-operator
flags such as --metrics-bind-address, --health-probe-bind-address,
--webhook-port, --webhook-cert-dir, --leader-elect, --perses-default-base-image,
--perses-server-url, or --enable-http2 if you need equivalent behavior, and rely
on the Perses CRD spec.tls for TLS configuration instead of operator CLI flags.
In `@go.mod`:
- Line 31: The go.mod has k8s.io/apiextensions-apiserver pinned to v0.35.2 while
other direct k8s.io modules (e.g., k8s.io/apiserver, k8s.io/client-go,
k8s.io/apimachinery) are at v0.35.4; update the k8s.io/apiextensions-apiserver
module version to v0.35.4 in go.mod to align patch versions and then run a
module sync (e.g., go get k8s.io/apiextensions-apiserver@v0.35.4 && go mod tidy)
to update the lockfile and ensure consistent dependency resolution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 432cdfbf-74d1-4b3f-87f7-5774d425d721
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
Makefilebundle/manifests/observability-operator.clusterserviceversion.yamlbundle/manifests/perses.dev_perses.yamlbundle/manifests/perses.dev_persesdashboards.yamlbundle/manifests/perses.dev_persesdatasources.yamlbundle/manifests/perses.dev_persesglobaldatasources.yamldeploy/perses/crds/perses.dev_perses.yamldeploy/perses/crds/perses.dev_persesdashboards.yamldeploy/perses/crds/perses.dev_persesdatasources.yamldeploy/perses/crds/perses.dev_persesglobaldatasources.yamldeploy/perses/perses-operator-cluster-role.yamldeploy/perses/perses-operator-deployment.yamlgo.modpkg/controllers/uiplugin/accelerators.gopkg/controllers/uiplugin/apm.gopkg/controllers/uiplugin/monitoring.go
✅ Files skipped from review due to trivial changes (4)
- deploy/perses/perses-operator-deployment.yaml
- deploy/perses/perses-operator-cluster-role.yaml
- pkg/controllers/uiplugin/apm.go
- deploy/perses/crds/perses.dev_persesdatasources.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- Makefile
- pkg/controllers/uiplugin/accelerators.go
- bundle/manifests/perses.dev_persesdashboards.yaml
- pkg/controllers/uiplugin/monitoring.go
- bundle/manifests/perses.dev_persesglobaldatasources.yaml
jgbernalp
force-pushed
the
update-perses-components
branch
from
57f75e3 to
bb76f98
Compare
jgbernalp
force-pushed
the
update-perses-components
branch
from
bb76f98 to
9023edd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/perses/crds/perses.dev_persesdatasources.yaml`:
- Around line 391-395: The tightened v1alpha2 schema adds three breaking
checks—top-level required: [spec], minLength: 1 on name/namespace, and
x-kubernetes-list-type: map on status.conditions—so audit and mitigate migration
impact by (1) scanning existing CRs for missing spec or empty-string
name/namespace and updating them (or provide a one-time migration job/conversion
webhook) so they pass required: [spec] and minLength constraints, (2) if empty
strings are legitimately used for type == "file" implement conditional
validation or relax the minLength rule for name/namespace when type == "file",
and (3) confirm controller status updates remain compatible with the switch to
x-kubernetes-list-type: map (status conditions produced by
meta.SetStatusCondition() are fine); reference the schema rules required:
[spec], minLength: 1 on name/namespace, and x-kubernetes-list-type: map for
status.conditions when making code or migration changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b3f5b22d-5dae-4122-a4e2-c137f67c01b0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
Makefilebundle/manifests/observability-operator.clusterserviceversion.yamlbundle/manifests/perses.dev_perses.yamlbundle/manifests/perses.dev_persesdashboards.yamlbundle/manifests/perses.dev_persesdatasources.yamlbundle/manifests/perses.dev_persesglobaldatasources.yamldeploy/perses/crds/perses.dev_perses.yamldeploy/perses/crds/perses.dev_persesdashboards.yamldeploy/perses/crds/perses.dev_persesdatasources.yamldeploy/perses/crds/perses.dev_persesglobaldatasources.yamldeploy/perses/perses-operator-cluster-role.yamldeploy/perses/perses-operator-deployment.yamlgo.modpkg/controllers/uiplugin/accelerators.gopkg/controllers/uiplugin/apm.gopkg/controllers/uiplugin/monitoring.go
✅ Files skipped from review due to trivial changes (4)
- Makefile
- deploy/perses/perses-operator-cluster-role.yaml
- deploy/perses/perses-operator-deployment.yaml
- pkg/controllers/uiplugin/monitoring.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/controllers/uiplugin/apm.go
- bundle/manifests/observability-operator.clusterserviceversion.yaml
- bundle/manifests/perses.dev_persesdashboards.yaml
- bundle/manifests/perses.dev_persesglobaldatasources.yaml
- go.mod
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
jgbernalp
force-pushed
the
update-perses-components
branch
from
9023edd to
a786528
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp, PeterYurkovich 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 |
2 participants
Fixes: https://redhat.atlassian.net/browse/COO-1555
This PR: