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 (jiajliu@redhat.com), 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 (jiajliu@redhat.com), 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 (jiajliu@redhat.com), 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 (jiajliu@redhat.com), 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 (jiajliu@redhat.com), 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 (jiajliu@redhat.com), 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 |
|
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 <cmurray@redhat.com>
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 (jiajliu@redhat.com), 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 (jiajliu@redhat.com), 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 (jiajliu@redhat.com), 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 (jiajliu@redhat.com), 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 (jiajliu@redhat.com), 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 <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
… prometheus Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
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 <cmurray@redhat.com>
|
/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. |
| // +kubebuilder:rbac:groups="apps",resources=deployments,verbs=get;list;watch | ||
| // +kubebuilder:rbac:groups=networking.k8s.io,resources=networkpolicies,verbs=get;list;watch | ||
| // +kubebuilder:rbac:groups="policy",resources=poddisruptionbudgets,verbs=get;list;watch | ||
| // +kubebuilder:rbac:groups=config.openshift.io,resources=images,verbs=get;list;watch |
There was a problem hiding this comment.
Don't we need this to support these calls:
$ git grep -B1 'ImageConfigName'
controllers/mapper.go-// Map will return a reconcile request for a UpdateService if the event is for a
controllers/mapper.go:// ImageConfigName Image or a ConfigMap referenced by AdditionalTrustedCA.Name.
--
controllers/mapper.go- image := &apicfgv1.Image{}
controllers/mapper.go: err := m.client.Get(ctx, types.NamespacedName{Name: defaults.ImageConfigName, Namespace: ""}, image)
...
controllers/updateservice_controller.go- image := &apicfgv1.Image{}
controllers/updateservice_controller.go: err := r.Client.Get(ctx, types.NamespacedName{Name: defaults.ImageConfigName}, image)
...
vendor/github.com/openshift/cluster-image-registry-operator/pkg/defaults/defaults.go-
vendor/github.com/openshift/cluster-image-registry-operator/pkg/defaults/defaults.go: ImageConfigName = "cluster"
| @@ -0,0 +1,23 @@ | |||
| package semver | |||
There was a problem hiding this comment.
nit for future pulls: having vendor bumps and things in the same commits as the human-generated content you added to main.go and similar makes it harder to review your changes (where I have to think more 😅) while excluding vendor/ and other automated changes (where I have to think about go.mod, but then can trust go mod ... tooling to have go.sum and vendor/ updated to match those go.mod changes, without reviewing vendor/ line by line).
| // TLSConfigFromCluster uses library-go's TLS security profile observer to | ||
| // read the cluster's APIServer configuration and returns a function that | ||
| // configures a tls.Config accordingly. If the APIServer resource is not | ||
| // found, the Intermediate profile is used as default. |
There was a problem hiding this comment.
I'm a bit nervous about how this will work vs. version skew. The OpenShift Update Service operator is a rolling stream operator, where:
New releases are required to support all in-maintenance OpenShift Container Platform versions...
which currently means that new OSUS could be installed on 4.12 OCP clusters. Checking on:
api$ git diff origin/release-4.12..origin/master -- config/v1/types_tlssecurityprofile.goturned up things like openshift/api#2697 removing ciphers like DHE-RSA-AES128-GCM-SHA256 because of lacking Go support, which... I guess there's nothing we can do about that. Other than the removal, the other change seems to be the addition of groups in 5.0, but that's optional, so it should be fine for a new OSUS to find it unset in a 4.22 or older cluster.
| reconciles local UpdateServices into more fundamental Kubernetes\nand OpenShift | ||
| resources like Cincinnati Deployments and Routes, and it reports the status | ||
| of those components in \nthe UpdateService status.\n" | ||
| operatorframework.io/cluster-monitoring: "true" |
There was a problem hiding this comment.
The
operatorframework.io/cluster-monitoring=trueannotation. When this annotation is set totrue, the OpenShift Console will update the namespace that the operator is being deployed to with theopenshift.io/cluster-monitoring=truelabel. When this annotation is present, the UI will update the OpenShift Monitoring Prometheus Operator ServiceAccount with the appropriate RBAC privileges for the given namespace as well, allowing operators to be scraped by the OpenShift Monitoring Prometheus Operator.
I'm not sure if that's still current, or how it would work on clusters where the console is not installed. but it suggests that 6cade1a might not be necessary.
| ] | ||
| capabilities: Basic Install | ||
| createdAt: "2026-06-17T20:26:03Z" | ||
| console.openshift.io/operator-monitoring-default: "true" |
There was a problem hiding this comment.
Are there docs for this one? It sounds a lot like operatorframework.io/cluster-monitoring, but I haven't found anything that looks like docs. I did find this code, but that also sounds a lot like the operatorframework.io/cluster-monitoring semantics. Having a commit message that says "hey, reviewer and future devs, these changes are the appropiate changes to make because..." would help convince me ;) But commit message or not, I'd like to understand what this annotation is about, and I haven't been able to turn up docs around it on my own :/
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.