ACM-37236: feat: onboard Thanos Operator into COO - #1151
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Thanos operator support through dependency and scheme registration, CRD generation, Kubernetes deployment and RBAC manifests, image configuration, and OLM packaging. The change adds five Thanos resource types, editor and viewer roles, manager permissions, leader-election permissions, and bundle metadata. Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
deploy/thanos/thanos-operator-deployment.yaml (1)
24-55: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd pod-level
securityContextfor defense-in-depth.The container-level security context is good (
allowPrivilegeEscalation: false, drop ALL capabilities). Adding a pod-levelseccompProfileandrunAsNonRootstrengthens the posture. Note: on OpenShift, the restricted SCC may already enforcerunAsNonRoot, so verify before setting it explicitly.🔒 Suggested pod securityContext
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers:🤖 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 `@deploy/thanos/thanos-operator-deployment.yaml` around lines 24 - 55, The `thanos-operator` workload only defines a container-level `securityContext`; add a pod-level `securityContext` on the `spec` for defense-in-depth. In `thanos-operator-deployment.yaml`, update the `spec` for the `thanos-operator` deployment to include a pod `seccompProfile` and, if compatible with your cluster policy, `runAsNonRoot` alongside the existing `containers` block. Keep the existing container `securityContext` as-is and ensure the new pod-level settings are applied consistently with OpenShift/SCC behavior.
🤖 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 `@cmd/operator/main.go`:
- Line 62: The defaultImages entry for "thanos-operator" is using a mutable
:latest image tag, which should be replaced with a pinned version to keep
deployments reproducible. Update the thanos-operator image in the defaultImages
map in main.go to a specific version tag consistent with the other pinned
images, using the existing "thanos-operator" key as the location to change.
In `@deploy/thanos/thanos-operator-cluster-role-binding.yaml`:
- Around line 13-14: The ClusterRoleBinding subject for the thanos-operator
ServiceAccount is missing the required namespace, so the binding cannot resolve
correctly. Update the ServiceAccount subject in the ClusterRoleBinding manifest
to include an explicit namespace, using the thanos-operator binding definition
so the role maps to the intended ServiceAccount. If the namespace must vary by
environment, apply it through a kustomize patch or variable substitution rather
than relying on namespace transformation.
In `@deploy/thanos/thanos-operator-deployment.yaml`:
- Around line 56-63: The `openshift-service-ca` volume is declared in the Thanos
operator deployment but never mounted, so either add a matching `volumeMounts`
entry on the relevant container that needs the CA cert or remove the unused
volume block entirely. Use the `volumes` definition and the container spec in
`thanos-operator-deployment.yaml` to keep the config consistent and ensure the
operator can actually access `service-ca.crt` if TLS support is required.
---
Nitpick comments:
In `@deploy/thanos/thanos-operator-deployment.yaml`:
- Around line 24-55: The `thanos-operator` workload only defines a
container-level `securityContext`; add a pod-level `securityContext` on the
`spec` for defense-in-depth. In `thanos-operator-deployment.yaml`, update the
`spec` for the `thanos-operator` deployment to include a pod `seccompProfile`
and, if compatible with your cluster policy, `runAsNonRoot` alongside the
existing `containers` block. Keep the existing container `securityContext` as-is
and ensure the new pod-level settings are applied consistently with
OpenShift/SCC behavior.
🪄 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: b5fd7423-dee8-4305-b2f3-a80cdfdc149a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
Makefilecmd/operator/main.godeploy/olm/bases/observability-operator.clusterserviceversion.yamldeploy/olm/kustomization.yamldeploy/thanos/crds/kustomization.yamldeploy/thanos/crds/monitoring.thanos.io_thanoscompacts.yamldeploy/thanos/crds/monitoring.thanos.io_thanosqueries.yamldeploy/thanos/crds/monitoring.thanos.io_thanosreceives.yamldeploy/thanos/crds/monitoring.thanos.io_thanosrulers.yamldeploy/thanos/crds/monitoring.thanos.io_thanosstores.yamldeploy/thanos/kustomization.yamldeploy/thanos/thanos-operator-cluster-role-binding.yamldeploy/thanos/thanos-operator-cluster-role.yamldeploy/thanos/thanos-operator-deployment.yamldeploy/thanos/thanos-operator-service-account.yamldeploy/thanos/thanoscompact_editor_role.yamldeploy/thanos/thanoscompact_viewer_role.yamldeploy/thanos/thanosquery_editor_role.yamldeploy/thanos/thanosquery_viewer_role.yamldeploy/thanos/thanosreceive_editor_role.yamldeploy/thanos/thanosreceive_viewer_role.yamldeploy/thanos/thanosruler_editor_role.yamldeploy/thanos/thanosruler_viewer_role.yamldeploy/thanos/thanosstore_editor_role.yamldeploy/thanos/thanosstore_viewer_role.yamlgo.modpkg/operator/scheme.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
deploy/thanos/thanos-operator-deployment.yaml (1)
56-63: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winVolume
openshift-service-castill declared but never mounted.The previous review flagged this issue and it remains unresolved: the
openshift-service-cavolume is defined but no container in the spec has a correspondingvolumeMountsentry. Either add the mount so the operator can access the CA cert, or remove the unused volume block.🔧 Proposed fix: add volumeMount to container
containers: - name: thanos-operator image: quay.io/thanos/thanos-operator:main-2026-07-02-e6d1790 args: - --leader-elect - --health-probe-bind-address=:8081 + volumeMounts: + - name: openshift-service-ca + mountPath: /etc/service-ca + readOnly: true securityContext:🤖 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 `@deploy/thanos/thanos-operator-deployment.yaml` around lines 56 - 63, The openshift-service-ca volume is declared in the deployment spec but never used by any container. Update the Thanos operator pod spec so the relevant container includes a matching volumeMount for openshift-service-ca, or remove the volume block entirely if the CA cert is not needed. Use the existing openshift-service-ca volume definition and the container spec in this deployment manifest to make the fix consistent.
🤖 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.
Duplicate comments:
In `@deploy/thanos/thanos-operator-deployment.yaml`:
- Around line 56-63: The openshift-service-ca volume is declared in the
deployment spec but never used by any container. Update the Thanos operator pod
spec so the relevant container includes a matching volumeMount for
openshift-service-ca, or remove the volume block entirely if the CA cert is not
needed. Use the existing openshift-service-ca volume definition and the
container spec in this deployment manifest to make the fix consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 11cb96fe-f22d-4bb5-b0fd-15e52a08de8a
📒 Files selected for processing (2)
cmd/operator/main.godeploy/thanos/thanos-operator-deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/operator/main.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@bundle/manifests/observability-operator.clusterserviceversion.yaml`:
- Line 129: The CSV still contains a placeholder user-facing description, so
update the operator description in the source CSV manifest used to generate this
bundle and regenerate the bundle afterward. Fix the `description` field in
`observability-operator.clusterserviceversion.yaml` under the OLM base manifest,
since that is what feeds OperatorHub/console metadata.
- Around line 1356-1366: The shipped CSV metadata still contains template
placeholders for the maintainer, provider, and link fields. Update the source
CSV in deploy/olm/bases/observability-operator.clusterserviceversion.yaml to
restore the real maintainer/provider/link values, then regenerate the bundle so
observability-operator.clusterserviceversion.yaml picks up the correct metadata.
🪄 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: 766f27ad-7336-428f-9b4f-9c87ac0aeee9
📒 Files selected for processing (25)
Makefilebundle.Dockerfilebundle/manifests/monitoring.thanos.io_thanoscompacts.yamlbundle/manifests/monitoring.thanos.io_thanosqueries.yamlbundle/manifests/monitoring.thanos.io_thanosreceives.yamlbundle/manifests/monitoring.thanos.io_thanosrulers.yamlbundle/manifests/monitoring.thanos.io_thanosstores.yamlbundle/manifests/observability-operator.clusterserviceversion.yamlbundle/manifests/thanoscompact-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/thanoscompact-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/thanosquery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/thanosquery-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/thanosreceive-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/thanosreceive-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/thanosruler-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/thanosruler-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/thanosstore-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/thanosstore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/metadata/annotations.yamldeploy/thanos/crds/monitoring.thanos.io_thanoscompacts.yamldeploy/thanos/crds/monitoring.thanos.io_thanosqueries.yamldeploy/thanos/crds/monitoring.thanos.io_thanosreceives.yamldeploy/thanos/crds/monitoring.thanos.io_thanosrulers.yamldeploy/thanos/crds/monitoring.thanos.io_thanosstores.yamldeploy/thanos/thanos-operator-deployment.yaml
💤 Files with no reviewable changes (1)
- deploy/thanos/thanos-operator-deployment.yaml
✅ Files skipped from review due to trivial changes (3)
- bundle/manifests/thanosstore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
- bundle/manifests/thanosruler-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
- bundle/metadata/annotations.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
02f6f71 to
cab046c
Compare
coleenquadros
changed the title
|
@coleenquadros: This pull request references ACM-37236 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 task 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. |
danielmellado
left a comment
danielmellado
left a comment
There was a problem hiding this comment.
Overall this LGTM, but I think that there may be a few issues. See inline comments. Thanks!
|
We have future work (https://redhat.atlassian.net/browse/COO-1969) to remove all second-level operators (e.g. Prometheus, Perses, ...) from the OLM CSV because it proved to be too inflexible. I suggest that we apply this decision to Thanos in advance. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: coleenquadros 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
I thought about the same, but I perhaps its better to not delay/block the thanos addition on that? We don't have a fix for all issues the bundle removal will cause and I doubt adding yet another operator to migrate adds significant overhead. That is unless Coleen is interested in the work needed for COO-1969. I'd be happy to have this here, though I wouldn't want to block this on COO-1969. |
jan--f
left a comment
jan--f
left a comment
There was a problem hiding this comment.
Overall this lgtm, except for one nit:
Some RBAC releated resources specify namespace: default, but the operator deployment doesn't. Given that there might a leader election iiuc this could fail if someone deploys via the kustomization file. The operator deployment could end up in a different namespace then the SA and CRB allowing the leader election.
The easiest would be to set a namespace in kustomization.yaml
We do this for prometheus operator here:
Add Thanos Operator (monitoring.thanos.io/v1alpha1) deployment artifacts following the Perses operator onboarding pattern: - 5 CRDs: ThanosQuery, ThanosReceive, ThanosCompact, ThanosStore, ThanosRuler - Operator deployment, ClusterRole, ServiceAccount, ClusterRoleBinding - 10 user-facing editor/viewer ClusterRoles - Wired into OLM bundle via deploy/olm/kustomization.yaml - CRDs registered in CSV as owned resources - Go dependency added and types registered in scheme - Makefile target for CRD generation (generate-thanos-op-crds) - Default image entry in cmd/operator/main.go Resolves: ACM-37236 Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
Regenerate Thanos CRDs with controller-gen maxDescLen=0 to reduce bundle size from ~1.2MB to ~590KB compressed (OLM limit is 1MB). CRD validation still works — only kubectl explain help text is removed. This is a trade-off to stay within the OLM bundle size constraint caused by the Thanos CRDs embedding full Kubernetes PodSpec schemas. Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
…naming The onboarded ClusterRole was missing coordination.k8s.io/leases and events permissions required by controller-runtime's default leader election, which would have caused the operator to crash-loop on startup. Add these via a dedicated leader-election Role/RoleBinding, matching upstream thanos-community/thanos-operator's own split between a cluster-scoped manager-role and a namespaced leader-election-role. Also rename the manager ClusterRole/ClusterRoleBinding and the ten CRD editor/viewer ClusterRoles to carry the thanos-operator- prefix, matching the naming used by the real rhobs staging deployment and the multicluster-observability-addon chart. Regenerate the OLM bundle to propagate the new permissions into the CSV's install strategy, since OLM installs from the bundle rather than the raw kustomize manifests. Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
0edecc6 to
1b609ff
Compare
Set explicit namespace to ensure Deployment and RBAC resources are deployed to the same namespace, preventing leader election failures. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
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. |
5 participants
Summary
ACM-37236
Onboard the Thanos Operator into COO following the same pattern as the Perses operator onboarding.
deploy/thanos/directory with 5 CRDs (monitoring.thanos.io/v1alpha1), operator deployment, RBAC, and user-facing editor/viewer rolesdeploy/olm/kustomization.yamlgithub.com/thanos-community/thanos-operatorand register types in schemegenerate-thanos-op-crdsMakefile targetthanos-operatorimage incmd/operator/main.goCRDs added
API support level:
TechPreviewNoUpgradePattern followed
Same as Perses operator onboarding (commit 5ba0330):
pkg/operator/scheme.gounder OpenShift feature gateJira: ACM-37236
Test plan
go build ./...passeskustomize build deploy/olm/renders all Thanos resources correctlygo test ./...unit tests pass (e2e skipped — requires live cluster)🤖 Generated with Claude Code