OCPBUGS-60905, OCPBUGS-51196, OCPBUGS-51181: fix(metrics): metrics export through service works now#268
OCPBUGS-60905, OCPBUGS-51196, OCPBUGS-51181: fix(metrics): metrics export through service works now#268Cali0707 wants to merge 6 commits into
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@Cali0707: This pull request references Jira Issue OCPBUGS-60905, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
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:
WalkthroughThe operator metrics endpoint transitions from plain HTTP on port 8080 to HTTPS on port 8443, with TLS certificates mounted into the application. Service and Prometheus ServiceMonitor labels are renamed for consistent discovery, the auth proxy service is activated in Kustomization, RBAC bindings are updated to use the correct service account, and the controller reconciles Prometheus Role and RoleBinding resources. Integration testing validates the entire flow. ChangesSecure Prometheus Metrics with RBAC
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings, 1 inconclusive)
✅ Passed checks (11 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 |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@wking: This pull request references Jira Issue OCPBUGS-60905, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/manager/manager.yaml (1)
29-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden the operator container policy defaults.
At Line 29 onward, the container still lacks explicit
securityContexthardening (runAsNonRoot,allowPrivilegeEscalation: false,readOnlyRootFilesystem) and explicit CPU/memory limits. This can violate cluster policy and weaken runtime isolation.As per coding guidelines, "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false" and "Resource limits (cpu, memory) on every container" are required for Kubernetes manifests.
Suggested patch
containers: - name: updateservice-operator @@ + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL + resources: + requests: + cpu: 50m + memory: 128Mi + limits: + cpu: 500m + memory: 512Mi🤖 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/manager/manager.yaml` around lines 29 - 56, The updateservice-operator container definition lacks required security hardening and resource limits. Add a securityContext field to the container spec with runAsNonRoot set to true, allowPrivilegeEscalation set to false, and readOnlyRootFilesystem set to true. Additionally, add a resources field with CPU and memory limits to comply with Kubernetes manifest coding guidelines and enforce runtime isolation. These should be added as direct properties of the container specification within the spec.template.spec.containers array.Sources: Coding guidelines, Linters/SAST tools
🤖 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.
Outside diff comments:
In `@config/manager/manager.yaml`:
- Around line 29-56: The updateservice-operator container definition lacks
required security hardening and resource limits. Add a securityContext field to
the container spec with runAsNonRoot set to true, allowPrivilegeEscalation set
to false, and readOnlyRootFilesystem set to true. Additionally, add a resources
field with CPU and memory limits to comply with Kubernetes manifest coding
guidelines and enforce runtime isolation. These should be added as direct
properties of the container specification within the
spec.template.spec.containers array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 075a3fb7-8292-4360-b98d-6e3d5cf626b9
📒 Files selected for processing (7)
bundle/manifests/update-service-operator.clusterserviceversion.yamlconfig/manager/manager.yamlconfig/prometheus/monitor.yamlconfig/rbac/auth_proxy_role_binding.yamlconfig/rbac/auth_proxy_service.yamlconfig/rbac/kustomization.yamlmain.go
🚧 Files skipped from review as they are similar to previous changes (2)
- config/rbac/auth_proxy_service.yaml
- config/prometheus/monitor.yaml
cec294b to
f893143
Compare
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 (2)
config/manager/manager.yaml (2)
29-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit container
securityContexthardening.Line 29 configures the workload container but does not set
runAsNonRoot,allowPrivilegeEscalation: false,readOnlyRootFilesystem, or capability drops. This leaves pod security posture dependent on cluster defaults.Suggested patch
containers: - name: updateservice-operator image: controller:latest imagePullPolicy: Always + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALLAs per coding guidelines, "
securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false" and "Drop ALL capabilities, add only what is required".🤖 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/manager/manager.yaml` around lines 29 - 52, Add a securityContext field to the updateservice-operator container to enforce security hardening. The securityContext should include runAsNonRoot set to true, allowPrivilegeEscalation set to false, and readOnlyRootFilesystem set to true. Additionally, configure the securityContext to drop all Linux capabilities using the capabilities field with DROP: ["ALL"] and only add back any capabilities that are strictly required for the operator to function. This should be added at the same indentation level as the env, ports, and volumeMounts fields within the container specification.Sources: Coding guidelines, Linters/SAST tools
29-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefine container resources and health probes.
Line 29 configures the container without CPU/memory limits and without readiness/liveness probes, which weakens scheduling safety and recovery behavior under load or startup failures.
As per coding guidelines, "
Resource limits (cpu, memory) on every container" and "Liveness + readiness probes defined".🤖 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/manager/manager.yaml` around lines 29 - 52, The updateservice-operator container is missing resource limits and health probes required by coding guidelines. Add a resources section with cpu and memory requests and limits after the env section. Add both livenessProbe and readinessProbe sections after the ports section, configuring them to perform HTTP probes against the https port (8443) to ensure proper scheduling and recovery behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@config/manager/manager.yaml`:
- Around line 29-52: Add a securityContext field to the updateservice-operator
container to enforce security hardening. The securityContext should include
runAsNonRoot set to true, allowPrivilegeEscalation set to false, and
readOnlyRootFilesystem set to true. Additionally, configure the securityContext
to drop all Linux capabilities using the capabilities field with DROP: ["ALL"]
and only add back any capabilities that are strictly required for the operator
to function. This should be added at the same indentation level as the env,
ports, and volumeMounts fields within the container specification.
- Around line 29-52: The updateservice-operator container is missing resource
limits and health probes required by coding guidelines. Add a resources section
with cpu and memory requests and limits after the env section. Add both
livenessProbe and readinessProbe sections after the ports section, configuring
them to perform HTTP probes against the https port (8443) to ensure proper
scheduling and recovery behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bccc308f-2e9c-462a-9259-d0c3e56d6ba3
📒 Files selected for processing (8)
bundle/manifests/update-service-operator.clusterserviceversion.yamlbundle/manifests/updateservice-operator-metrics_v1_service.yamlconfig/manager/manager.yamlconfig/prometheus/monitor.yamlconfig/rbac/auth_proxy_role_binding.yamlconfig/rbac/auth_proxy_service.yamlconfig/rbac/kustomization.yamlmain.go
✅ Files skipped from review due to trivial changes (1)
- config/rbac/auth_proxy_service.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- bundle/manifests/update-service-operator.clusterserviceversion.yaml
- config/rbac/auth_proxy_role_binding.yaml
- main.go
- config/prometheus/monitor.yaml
- config/rbac/kustomization.yaml
Cali0707
changed the title
Cali0707
changed the title
|
/verified by @Cali0707, running a cluster bot with this PR checked out: |
openshift-ci-robot
added
the
verified
|
@Cali0707: This PR has been marked as verified by 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. |
|
/cc @wking |
openshift-ci-robot
removed
the
verified
Cali0707
changed the title
openshift-ci-robot
added
jira/invalid-bug
|
@Cali0707: This pull request references Jira Issue OCPBUGS-60905, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-51196, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-bug
|
@Cali0707: This pull request references Jira Issue OCPBUGS-60905, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. This pull request references Jira Issue OCPBUGS-51196, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
/jira refresh |
|
@Cali0707: This pull request references Jira Issue OCPBUGS-60905, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. This pull request references Jira Issue OCPBUGS-51196, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. 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. |
|
/verified by @Cali0707, curling the metrics svc on a cluster bot cluster |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Cali0707, shahsahil264 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 |
|
Looks like I need to figure out how to get auth for the Prometheus instance within this job properly... |
cbca6c4 to
8613e58
Compare
|
New changes are detected. LGTM label has been removed. |
9535fbc to
3bab9a7
Compare
|
/cc @wking Lmk what else is needed here |
| OperatorNamespace string | ||
| } | ||
|
|
||
| // +kubebuilder:rbac:groups="",resources=configmaps;pods;services;secrets,verbs=get;list;watch |
There was a problem hiding this comment.
| // +kubebuilder:rbac:groups="",resources=namespaces;configmaps;pods;services;secrets,verbs=get;list;watch |
| ns.Labels = map[string]string{} | ||
| } | ||
| if ns.Labels["openshift.io/cluster-monitoring"] != "true" { | ||
| ns.Labels["openshift.io/cluster-monitoring"] = "true" |
There was a problem hiding this comment.
IIUC, this is not done by the controller in production, causing https://redhat.atlassian.net/browse/OCPBUGS-51181.
There was a problem hiding this comment.
I think the issue is we were missing some operator CSV labels - my understanding is that the console looks at those to determine what should be shown: https://github.com/openshift/console/blob/6d0dcf1d76337e46d95ea1b546d5ffe692d7918d/frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx#L225-L226
The console actually labels the ns itself
That said, I added the required labels into this branch 😄
Signed-off-by: Calum Murray <[email protected]>
3bab9a7 to
f9ca058
Compare
Cali0707
changed the title
openshift-ci-robot
added
jira/invalid-bug
|
@Cali0707: This pull request references Jira Issue OCPBUGS-60905, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-51196, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-51181, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@Cali0707: This pull request references Jira Issue OCPBUGS-60905, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. This pull request references Jira Issue OCPBUGS-51196, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. This pull request references Jira Issue OCPBUGS-51181, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. 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. |
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
… prometheus Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
dba7170 to
da505c9
Compare
2uasimojo
left a comment
2uasimojo
left a comment
There was a problem hiding this comment.
The TLS discovery bits seem sane to me (FWIW).
|
|
||
| observedConfig, errs := apiserver.ObserveTLSSecurityProfile(listers, recorder, map[string]interface{}{}) | ||
| if len(errs) > 0 { | ||
| return nil, fmt.Errorf("observing TLS security profile: %v", errs[0]) |
There was a problem hiding this comment.
Consider errors.Wrap() (throughout).
Signed-off-by: Calum Murray <[email protected]>
|
/test install-bundle |
|
@Cali0707: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
The service monitor and service for metrics did not match the labels on the deployment correctly. This seems to date back to when they were first added.
Summary by CodeRabbit
New Features
8443, including TLS certificate mounting.Bug Fixes
ServiceMonitorselection to match the metrics Service using the correct label key.Chores
:8443.