feat: have CVO inject the centralized TLS configuration into the operator's config#1167
Open
ingvagabund wants to merge 1 commit into
Open
feat: have CVO inject the centralized TLS configuration into the operator's config#1167ingvagabund wants to merge 1 commit into
ingvagabund wants to merge 1 commit into
Conversation
…ator's config Also, have the operator restart whenever the config changes.
WalkthroughThis PR configures TLS injection and file monitoring for the console-operator. The ConfigMap annotation enables TLS secret injection, while both Deployment manifests are updated with file-watching arguments to gracefully restart the operator when controller config or TLS certificate files change. ChangesTLS Injection and File Watching Configuration
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ingvagabund 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
manifests/07-operator-ibm-cloud-managed.yaml (1)
28-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify operator behavior when watching files in optional secret.
Same concern as in
manifests/07-operator.yaml: theserving-certsecret is optional (line 105), but file-watching is configured for TLS files within it. Verify the operator gracefully handles watching non-existent files at startup before CVO injects the TLS secret.🤖 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 `@manifests/07-operator-ibm-cloud-managed.yaml` around lines 28 - 30, The operator manifest configures file-watching flags (--terminate-on-files) for TLS files inside the optional "serving-cert" secret (the paths for tls.crt/tls.key) which may not exist at startup; update the operator startup logic or container args so the operator handles missing files gracefully by either removing those --terminate-on-files entries for the serving-cert files when the secret is optional or adding a startup check that detects absence of the serving-cert files and skips/defers termination-watching until the secret is present (ensure the code or entrypoint that parses the --terminate-on-files flags tolerates non-existent paths and does not exit on startup).
🧹 Nitpick comments (2)
manifests/07-operator.yaml (1)
85-88: ⚡ Quick winConsider adding resource limits alongside requests.
The container specifies resource requests but no limits. While requests are sufficient for scheduling, adding limits helps prevent resource exhaustion and improves cluster stability. As per coding guidelines, resource limits (cpu, memory) should be present on every container.
📊 Suggested resource limits
resources: requests: memory: "100Mi" cpu: "10m" + limits: + memory: "200Mi" + cpu: "100m"🤖 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 `@manifests/07-operator.yaml` around lines 85 - 88, The manifest defines resource requests under resources.requests (memory: "100Mi", cpu: "10m") but no resource limits; add a resources.limits block next to requests and set appropriate cpu and memory caps (e.g., cpu: "100m", memory: "200Mi") to prevent resource exhaustion and comply with the guideline—update the container spec that contains the resources key to include limits for both cpu and memory.manifests/07-operator-ibm-cloud-managed.yaml (1)
63-66: ⚡ Quick winConsider adding resource limits alongside requests.
The container specifies resource requests but no limits. Adding limits helps prevent resource exhaustion. As per coding guidelines, resource limits (cpu, memory) should be present on every container.
Note: Since this is an auto-generated file (line 1), the change should likely be made in the source template or generator that produces this manifest.
📊 Suggested resource limits
resources: requests: cpu: 10m memory: 100Mi + limits: + cpu: 100m + memory: 200Mi🤖 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 `@manifests/07-operator-ibm-cloud-managed.yaml` around lines 63 - 66, The manifest defines resources.requests (cpu: 10m, memory: 100Mi) but no resources.limits; update the source Helm/template/generator that produces this manifest to add resources.limits with appropriate cpu and memory values (e.g., limits slightly above requests) so every container has both requests and limits; modify the template field under resources to include limits (cpu and memory) alongside the existing requests to ensure the generated manifests include limits going forward.
🤖 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 `@manifests/07-operator.yaml`:
- Around line 61-63: The args
"--terminate-on-files=/var/run/secrets/serving-cert/tls.crt" and
"--terminate-on-files=/var/run/secrets/serving-cert/tls.key" in the operator
container args cause a terminate() on the first appearance event (e.g. when an
optional serving-cert secret is later created), so either remove these two
"--terminate-on-files=..." entries from the args list in 07-operator.yaml or
make the serving-cert secret mount non-optional (ensure the files exist at
startup); choose the approach you want and update the container args (or the
secret/mount) accordingly to prevent an unintended pod restart when
tls.crt/tls.key appear.
---
Duplicate comments:
In `@manifests/07-operator-ibm-cloud-managed.yaml`:
- Around line 28-30: The operator manifest configures file-watching flags
(--terminate-on-files) for TLS files inside the optional "serving-cert" secret
(the paths for tls.crt/tls.key) which may not exist at startup; update the
operator startup logic or container args so the operator handles missing files
gracefully by either removing those --terminate-on-files entries for the
serving-cert files when the secret is optional or adding a startup check that
detects absence of the serving-cert files and skips/defers termination-watching
until the secret is present (ensure the code or entrypoint that parses the
--terminate-on-files flags tolerates non-existent paths and does not exit on
startup).
---
Nitpick comments:
In `@manifests/07-operator-ibm-cloud-managed.yaml`:
- Around line 63-66: The manifest defines resources.requests (cpu: 10m, memory:
100Mi) but no resources.limits; update the source Helm/template/generator that
produces this manifest to add resources.limits with appropriate cpu and memory
values (e.g., limits slightly above requests) so every container has both
requests and limits; modify the template field under resources to include limits
(cpu and memory) alongside the existing requests to ensure the generated
manifests include limits going forward.
In `@manifests/07-operator.yaml`:
- Around line 85-88: The manifest defines resource requests under
resources.requests (memory: "100Mi", cpu: "10m") but no resource limits; add a
resources.limits block next to requests and set appropriate cpu and memory caps
(e.g., cpu: "100m", memory: "200Mi") to prevent resource exhaustion and comply
with the guideline—update the container spec that contains the resources key to
include limits for both cpu and memory.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 28c384d0-ae3d-4587-b0f8-a33acfbf3871
📒 Files selected for processing (3)
manifests/05-config.yamlmanifests/07-operator-ibm-cloud-managed.yamlmanifests/07-operator.yaml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.yaml
⚙️ CodeRabbit configuration file
**/*.yaml: Review YAML manifests based on content and kind.Refer to /manifest-review when YAML contains:
kind: Roleorkind: ClusterRole(RBAC review)kind: RoleBindingorkind: ClusterRoleBindingannotations:section (check for cluster profiles)verbs: ["*"]or wildcard permissionsapiGroups: ["*"]or overly broad permissions- ServiceAccount references in subjects
Check for required annotations in manifests/:
include.release.openshift.io/hypershiftinclude.release.openshift.io/ibm-cloud-managedinclude.release.openshift.io/self-managed-high-availabilityinclude.release.openshift.io/single-node-developercapability.openshift.io/name: ConsoleFor quickstarts/, additionally check:
- QuickStart spec structure
- Task descriptions and prerequisites
- See quickstarts/README.md for guidelines
Files:
manifests/07-operator.yamlmanifests/05-config.yamlmanifests/07-operator-ibm-cloud-managed.yaml
**/*.{yaml,yml}
⚙️ CodeRabbit configuration file
**/*.{yaml,yml}: If this is a Kubernetes/OpenShift manifest or Helm template:
- securityContext: runAsNonRoot, readOnlyRootFilesystem,
allowPrivilegeEscalation: false- Drop ALL capabilities, add only what is required
- Resource limits (cpu, memory) on every container
- No hostPID, hostNetwork, hostIPC, privileged: true
- NetworkPolicy defined for the namespace
- OpenShift: SCC must be restricted or custom-scoped
- Liveness + readiness probes defined
- automountServiceAccountToken: false unless needed
- RBAC: least privilege; no cluster-admin for workloads
- Helm: no .Values interpolation in shell commands
Files:
manifests/07-operator.yamlmanifests/05-config.yamlmanifests/07-operator-ibm-cloud-managed.yaml
🔇 Additional comments (1)
manifests/05-config.yaml (1)
12-12: LGTM!
Comment on lines
+61
to
+63
| - "--terminate-on-files=/var/run/configmaps/config/controller-config.yaml" | ||
| - "--terminate-on-files=/var/run/secrets/serving-cert/tls.crt" | ||
| - "--terminate-on-files=/var/run/secrets/serving-cert/tls.key" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for file-watching implementation and error handling in the operator codebase
# Check for terminate-on-files flag implementation and how it handles missing files
rg -nP --type=go -C5 'terminate-on-files|TerminateOnFiles'
# Look for file watching logic and error handling
rg -nP --type=go -C3 'fsnotify|file.*watch|inotify'
# Check if there's logic to handle optional files or graceful degradation
rg -nP --type=go -C3 'optional.*file|file.*optional|skip.*missing'Repository: openshift/console-operator
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find fileobserver implementation =="
rg -n "package fileobserver" -S vendor openshift || true
rg -n "fileobserver\.NewObserver|NewObserver\(" -S vendor | head -n 50 || true
rg -n "AddReactor\(" -S vendor | head -n 50 || true
rg -n "TerminateOnFiles" -S . | head -n 50 || true
echo
echo "== Show relevant manifest sections =="
# Print a manageable chunk around the mentioned lines
sed -n '40,140p' manifests/07-operator.yaml | cat -nRepository: openshift/console-operator
Length of output: 6690
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== observer.go (fileobserver) =="
nl -ba vendor/github.com/openshift/library-go/pkg/controller/fileobserver/observer.go | sed -n '1,220p'
echo
echo "== observer_polling.go (fileobserver) =="
nl -ba vendor/github.com/openshift/library-go/pkg/controller/fileobserver/observer_polling.go | sed -n '1,240p'
echo
echo "== Any helper types in observer.go beyond 220 (if small) =="
nl -ba vendor/github.com/openshift/library-go/pkg/controller/fileobserver/observer.go | sed -n '220,420p'Repository: openshift/console-operator
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== observer.go (fileobserver) =="
sed -n '1,260p' vendor/github.com/openshift/library-go/pkg/controller/fileobserver/observer.go | cat -n
echo
echo "== observer_polling.go (fileobserver) =="
sed -n '1,320p' vendor/github.com/openshift/library-go/pkg/controller/fileobserver/observer_polling.go | cat -n
echo
echo "== observer.go tail (if larger) =="
sed -n '260,520p' vendor/github.com/openshift/library-go/pkg/controller/fileobserver/observer.go | cat -nRepository: openshift/console-operator
Length of output: 10870
🏁 Script executed:
sed -n '130,210p' vendor/github.com/openshift/library-go/pkg/controller/controllercmd/cmd.go | cat -nRepository: openshift/console-operator
Length of output: 3301
Clarify --terminate-on-files behavior with optional serving-cert
The below args still terminate the operator when the watched files change:
- "--terminate-on-files=/var/run/secrets/serving-cert/tls.crt"
- "--terminate-on-files=/var/run/secrets/serving-cert/tls.key"- The file observer is polling-based and treats missing files as
exists=false; it will not crash or exit just becausetls.crt/tls.keydon’t exist at startup. - However, the terminate reactor calls
terminate()on the first observed file event, so when the optionalserving-certsecret is later created and thetls.*files appear, the operator will terminate (causing a pod restart). - If a restart on secret injection/creation is not intended, adjust
terminate-on-filesfor these optional TLS paths (or make the secret non-optional).
🤖 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 `@manifests/07-operator.yaml` around lines 61 - 63, The args
"--terminate-on-files=/var/run/secrets/serving-cert/tls.crt" and
"--terminate-on-files=/var/run/secrets/serving-cert/tls.key" in the operator
container args cause a terminate() on the first appearance event (e.g. when an
optional serving-cert secret is later created), so either remove these two
"--terminate-on-files=..." entries from the args list in 07-operator.yaml or
make the serving-cert secret mount non-optional (ensure the files exist at
startup); choose the approach you want and update the container args (or the
secret/mount) accordingly to prevent an unintended pod restart when
tls.crt/tls.key appear.
Member
Author
|
/retest-required |
Contributor
|
@ingvagabund: 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. |
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
1 participant
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.
Also, have the operator restart whenever the config changes.
wip-docs: openshift/enhancements#2020
Analysis / Root cause:
Solution description:
Test setup:
Test cases:
Browser conformance:
Additional info:
Reviewers and assignees:
Summary by CodeRabbit