OCPEDGE-2346: feat(manager): read TLS config from API server#2105
OCPEDGE-2346: feat(manager): read TLS config from API server#2105qJkee wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@qJkee: This pull request references OCPEDGE-2346 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 story to target the "4.22.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. |
openshift-ci
bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qJkee 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 |
openshift-ci
bot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
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: Pro 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:
WalkthroughFetch API server TLSProfile at startup, build a tls.Config and inject it into manager and webhook TLS options, add a TLS-profile watcher that cancels the manager context on changes to trigger a restart, bump dependencies, and update CRD annotations and a test import. Changes
Sequence Diagram(s)sequenceDiagram
participant Supervisor as Process\nSupervisor
participant Manager as Manager
participant APIServer as API\nServer
participant TLSWatcher as TLS\nProfile Watcher
Supervisor->>Manager: Start manager process
Manager->>APIServer: GET TLSProfile (startup)
APIServer-->>Manager: TLSProfile
Manager->>Manager: Build tls.Config, set Manager & Webhook TLS options
Supervisor->>Manager: Launch manager with TLS config
Note over TLSWatcher,APIServer: Runtime monitoring
TLSWatcher->>APIServer: Watch TLSProfile resource
APIServer-->>TLSWatcher: Notify on TLSProfile change
TLSWatcher->>Manager: Log change, call cancel() (ErrTLSProfileModified)
Manager->>Supervisor: Exit/propagate error to allow restart/reload
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)
📝 Coding Plan
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 |
|
@qJkee: This pull request references OCPEDGE-2346 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 story to target the "4.22.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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2105 +/- ##
==========================================
+ Coverage 51.73% 52.11% +0.38%
==========================================
Files 52 52
Lines 3901 3901
==========================================
+ Hits 2018 2033 +15
+ Misses 1715 1701 -14
+ Partials 168 167 -1
🚀 New features to boost your workflow:
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/vgmanager/vgmanager.go`:
- Around line 197-205: The TLS watcher currently calls cancel() in
SecurityProfileWatcher.OnProfileChange which triggers an abnormal exit; define a
new sentinel error ErrTLSProfileModified, replace the cancel() call with
cancelWithCause(ErrTLSProfileModified) inside the OnProfileChange closure, and
update the existing shutdown/error-handling logic that checks for
ErrConfigModified to also check for ErrTLSProfileModified so TLS reloads exit
via the clean reload path (status 0) rather than the abnormal path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 671ed331-b528-4e67-84a7-1dc48d4d0fd0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
cmd/operator/operator.gocmd/vgmanager/vgmanager.gogo.mod
|
@qJkee: This pull request references OCPEDGE-2346 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 story to target the "4.22.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. |
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)
cmd/vgmanager/vgmanager.go (1)
303-312:⚠️ Potential issue | 🟠 MajorMissing
ErrTLSProfileModifiedcheck causes abnormal exit on TLS profile changes.The TLS watcher correctly calls
cancelWithCause(ErrTLSProfileModified)at line 205, but the error handling here doesn't check for this error. When a TLS profile change occurs:
cancelWithCause(ErrTLSProfileModified)is calledmgr.Start()returns- The code checks
ErrConfigModified(line 303) - no match- The code checks
ErrPluginRegistrationFailed(line 306) - no match- Falls through to
ctx.Err()check (line 309) - matches because context is cancelled- Logs "exiting abnormally" and calls
os.Exit(1)This results in an abnormal exit instead of the intended clean restart.
🐛 Proposed fix
if errors.Is(context.Cause(ctx), ErrConfigModified) { opts.SetupLog.Info("exiting pod due to modified configuration") os.Exit(0) + } else if errors.Is(context.Cause(ctx), ErrTLSProfileModified) { + opts.SetupLog.Info("exiting pod due to modified TLS profile") + os.Exit(0) } else if errors.Is(context.Cause(ctx), icsi.ErrPluginRegistrationFailed) { opts.SetupLog.Error(context.Cause(ctx), "exiting pod due to failed plugin registration") os.Exit(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/vgmanager/vgmanager.go` around lines 303 - 312, Add an explicit check for ErrTLSProfileModified in the shutdown handling so TLS-driven cancellations perform a clean restart: after the existing errors.Is(context.Cause(ctx), ErrConfigModified) branch, add a branch that checks errors.Is(context.Cause(ctx), ErrTLSProfileModified) and log a descriptive info message via opts.SetupLog (similar style to the ErrConfigModified branch) and call os.Exit(0); this aligns the handling of cancelWithCause(ErrTLSProfileModified) from the TLS watcher with the intended clean exit instead of falling through to the ctx.Err() abnormal-exit path.go.mod (1)
3-3:⚠️ Potential issue | 🟠 MajorUpdate Go version to a valid release.
Go 1.24.11 does not exist. The official Go releases list shows the latest available versions are go1.26.1 and go1.25.8. Update
go.modline 3 to specify a valid Go version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 3, The go.mod declares an invalid Go version ("go 1.24.11"); update the module's Go version to a valid release tag (for example "go 1.26" or "go 1.26.1") by replacing the incorrect version token in go.mod so the file specifies a real supported Go release.
🧹 Nitpick comments (1)
cmd/operator/operator.go (1)
244-257: Consider usingcancelWithCausefor consistency with vgmanager.The TLS watcher in
vgmanager.gousescancelWithCause(ErrTLSProfileModified)to enable specific handling of TLS profile changes (logging and clean exit viaos.Exit(0)). Here, using plaincancel()will trigger a shutdown, but the exit path differs:
- In
vgmanager.go: TLS profile changes are explicitly logged and exit cleanly.- In
operator.go: The function returnsnilaftermgr.Start(), which should result in a clean exit, but without explicit logging indicating the reason.This is acceptable if the pod restart mechanism handles both cases identically, but for observability and consistency, consider aligning with the vgmanager pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/operator/operator.go` around lines 244 - 257, Replace the plain context cancel in the TLS watcher callback with the cancel-with-cause pattern used in vgmanager: inside the OnProfileChange closure of SecurityProfileWatcher (where cancel() is called), invoke cancelWithCause(ErrTLSProfileModified) instead so the shutdown path includes the specific cancellation cause and matches vgmanager's logging/exit handling; ensure ErrTLSProfileModified and cancelWithCause are imported/available in this scope or propagate them into operator.go if needed.
🤖 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 `@cmd/vgmanager/vgmanager.go`:
- Around line 303-312: Add an explicit check for ErrTLSProfileModified in the
shutdown handling so TLS-driven cancellations perform a clean restart: after the
existing errors.Is(context.Cause(ctx), ErrConfigModified) branch, add a branch
that checks errors.Is(context.Cause(ctx), ErrTLSProfileModified) and log a
descriptive info message via opts.SetupLog (similar style to the
ErrConfigModified branch) and call os.Exit(0); this aligns the handling of
cancelWithCause(ErrTLSProfileModified) from the TLS watcher with the intended
clean exit instead of falling through to the ctx.Err() abnormal-exit path.
In `@go.mod`:
- Line 3: The go.mod declares an invalid Go version ("go 1.24.11"); update the
module's Go version to a valid release tag (for example "go 1.26" or "go
1.26.1") by replacing the incorrect version token in go.mod so the file
specifies a real supported Go release.
---
Nitpick comments:
In `@cmd/operator/operator.go`:
- Around line 244-257: Replace the plain context cancel in the TLS watcher
callback with the cancel-with-cause pattern used in vgmanager: inside the
OnProfileChange closure of SecurityProfileWatcher (where cancel() is called),
invoke cancelWithCause(ErrTLSProfileModified) instead so the shutdown path
includes the specific cancellation cause and matches vgmanager's logging/exit
handling; ensure ErrTLSProfileModified and cancelWithCause are
imported/available in this scope or propagate them into operator.go if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 936b55ad-2e88-4565-afd9-862ec05ae498
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/operator/operator.gocmd/vgmanager/vgmanager.gogo.modinternal/controllers/vgmanager/validatelvs_test.go
|
@qJkee: This pull request references OCPEDGE-2346 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 story to target the "4.22.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. |
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)
cmd/vgmanager/vgmanager.go (2)
303-315:⚠️ Potential issue | 🟡 MinorAdd explicit handling for normal signal-driven shutdown.
Normal
SIGTERMorSIGINTfromsignal.NotifyContextfalls through to the generic error branch, logging as "exiting abnormally" and exiting with code 1. This should be a clean shutdown.Suggested fix
cause := context.Cause(ctx) if errors.Is(cause, ErrConfigModified) { opts.SetupLog.Info("exiting pod due to modified configuration") os.Exit(0) } else if errors.Is(cause, ErrTLSProfileModified) { opts.SetupLog.Info("exiting pod due to modified TLS profile") os.Exit(0) } else if errors.Is(cause, icsi.ErrPluginRegistrationFailed) { opts.SetupLog.Error(cause, "exiting pod due to failed plugin registration") os.Exit(0) + } else if errors.Is(cause, context.Canceled) { + opts.SetupLog.Info("received shutdown signal") + return nil } else if err := ctx.Err(); err != nil { opts.SetupLog.Error(err, "exiting abnormally") os.Exit(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/vgmanager/vgmanager.go` around lines 303 - 315, The shutdown handler treats signal-driven context cancellation as an abnormal exit; update the branching after calling context.Cause(ctx) to explicitly handle normal signal shutdown by checking if ctx.Err() is context.Canceled (and optionally context.DeadlineExceeded if desired) and call opts.SetupLog.Info with a clear "exiting due to signal" message and os.Exit(0); keep the existing branches for ErrConfigModified, ErrTLSProfileModified, icsi.ErrPluginRegistrationFailed, and the fallback error branch that exits with code 1.
148-211:⚠️ Potential issue | 🔴 CriticalAdd RBAC rule for
config.openshift.io/apiserversto both vgmanager and operator service accounts.This PR adds
FetchAPIServerTLSProfile(startup read) andSecurityProfileWatcher(runtime watch) for the cluster-scoped APIServer resource to both vgmanager and operator. The respective RBAC ClusterRoles/Roles lack the required permissions (get,list,watchonapiservers.config.openshift.io), causing startup and runtime failures.
- Add to
config/rbac/vg_manager_clusterrole.yaml:- apiGroups: - config.openshift.io resources: - apiservers verbs: - get - list - watch- Add same rule to operator RBAC (
config/rbac/role.yamlor applicable operator ClusterRole if separate).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/vgmanager/vgmanager.go` around lines 148 - 211, The vgmanager and operator lack RBAC for the cluster-scoped Apiserver resource used by FetchAPIServerTLSProfile (startup read) and SecurityProfileWatcher (runtime watch); add a rule granting get, list, watch on apiGroups: ["config.openshift.io"] resources: ["apiservers"] to both the vgmanager ClusterRole (config/rbac/vg_manager_clusterrole.yaml) and the operator's RBAC (role/ClusterRole used by the operator service account) so the setupClient and TLS watcher can successfully read/watch the Apiserver resource at startup and runtime.cmd/operator/operator.go (1)
191-257:⚠️ Potential issue | 🔴 CriticalAdd RBAC for
config.openshift.io/apiserversaccess.Both
cmd/operator/operator.goandcmd/vgmanager/vgmanager.gocallFetchAPIServerTLSProfile(requiresget) and registerSecurityProfileWatcher(requireslist/watch) on the cluster-scopedAPIServerresource. The operator'smanager-roleClusterRole inconfig/rbac/role.yamllacks this rule and must be updated, or the startup will fail with403 Forbiddenat theFetchAPIServerTLSProfilecall.Add the following rule to
config/rbac/role.yaml:- apiGroups: - config.openshift.io resources: - apiservers verbs: - get - list - watch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/operator/operator.go` around lines 191 - 257, The operator currently calls FetchAPIServerTLSProfile and registers a SecurityProfileWatcher (see FetchAPIServerTLSProfile, SecurityProfileWatcher, tlsWatcherController) against the cluster-scoped APIServer resource but the manager-role ClusterRole in config/rbac/role.yaml lacks permissions; update config/rbac/role.yaml to add a rule that grants get, list, and watch on the config.openshift.io apiservers resource so FetchAPIServerTLSProfile and tlsWatcherController.SetupWithManager can succeed without 403 errors.
🤖 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 `@cmd/operator/operator.go`:
- Around line 191-257: The operator currently calls FetchAPIServerTLSProfile and
registers a SecurityProfileWatcher (see FetchAPIServerTLSProfile,
SecurityProfileWatcher, tlsWatcherController) against the cluster-scoped
APIServer resource but the manager-role ClusterRole in config/rbac/role.yaml
lacks permissions; update config/rbac/role.yaml to add a rule that grants get,
list, and watch on the config.openshift.io apiservers resource so
FetchAPIServerTLSProfile and tlsWatcherController.SetupWithManager can succeed
without 403 errors.
In `@cmd/vgmanager/vgmanager.go`:
- Around line 303-315: The shutdown handler treats signal-driven context
cancellation as an abnormal exit; update the branching after calling
context.Cause(ctx) to explicitly handle normal signal shutdown by checking if
ctx.Err() is context.Canceled (and optionally context.DeadlineExceeded if
desired) and call opts.SetupLog.Info with a clear "exiting due to signal"
message and os.Exit(0); keep the existing branches for ErrConfigModified,
ErrTLSProfileModified, icsi.ErrPluginRegistrationFailed, and the fallback error
branch that exits with code 1.
- Around line 148-211: The vgmanager and operator lack RBAC for the
cluster-scoped Apiserver resource used by FetchAPIServerTLSProfile (startup
read) and SecurityProfileWatcher (runtime watch); add a rule granting get, list,
watch on apiGroups: ["config.openshift.io"] resources: ["apiservers"] to both
the vgmanager ClusterRole (config/rbac/vg_manager_clusterrole.yaml) and the
operator's RBAC (role/ClusterRole used by the operator service account) so the
setupClient and TLS watcher can successfully read/watch the Apiserver resource
at startup and runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a92d242a-30fe-4e7e-a1ac-b0b4a0111384
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/operator/operator.gocmd/vgmanager/vgmanager.gogo.modinternal/controllers/vgmanager/validatelvs_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controllers/vgmanager/validatelvs_test.go
openshift-merge-robot
added
the
needs-rebase
|
@qJkee: This pull request references OCPEDGE-2346 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 story to target the "4.22.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. |
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/operator/operator.go (1)
142-142: Consider usingcontext.WithCancelCausefor consistency with vgmanager.The operator uses
context.WithCancelwhile vgmanager usescontext.WithCancelCause. When the TLS watcher callscancel()at Line 251, the shutdown reason isn't tracked. This works (returns nil fromrun), but differs from vgmanager's explicit cause-based exit handling.♻️ Suggested alignment with vgmanager pattern
func run(cmd *cobra.Command, _ []string, opts *Options) error { - ctx, cancel := context.WithCancel(cmd.Context()) - defer cancel() + ctx, cancelWithCause := context.WithCancelCause(cmd.Context()) + defer cancelWithCause(nil)Then update the TLS watcher callback (Line 251) to use
cancelWithCause(ErrTLSProfileModified)and add cause-based exit handling aftermgr.Start.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/operator/operator.go` at line 142, The code creates ctx and cancel via context.WithCancel (ctx, cancel := context.WithCancel(cmd.Context())) but should mirror vgmanager by using context.WithCancelCause so shutdown reasons are preserved; change to use context.WithCancelCause and replace the cancel variable with cancelWithCause (or similar), update the TLS watcher callback to call cancelWithCause(ErrTLSProfileModified) instead of cancel(), and after mgr.Start add cause-aware exit handling that inspects the cancel cause (ErrTLSProfileModified) to drive the appropriate shutdown path; reference the symbols ctx, cancel/cancelWithCause, ErrTLSProfileModified, the TLS watcher callback, and mgr.Start when making the edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/operator/operator.go`:
- Line 142: The code creates ctx and cancel via context.WithCancel (ctx, cancel
:= context.WithCancel(cmd.Context())) but should mirror vgmanager by using
context.WithCancelCause so shutdown reasons are preserved; change to use
context.WithCancelCause and replace the cancel variable with cancelWithCause (or
similar), update the TLS watcher callback to call
cancelWithCause(ErrTLSProfileModified) instead of cancel(), and after mgr.Start
add cause-aware exit handling that inspects the cancel cause
(ErrTLSProfileModified) to drive the appropriate shutdown path; reference the
symbols ctx, cancel/cancelWithCause, ErrTLSProfileModified, the TLS watcher
callback, and mgr.Start when making the edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 59b2567d-40f1-49a6-be48-690ac98fc702
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
bundle/manifests/lvm.topolvm.io_lvmclusters.yamlbundle/manifests/lvm.topolvm.io_lvmvolumegroupnodestatuses.yamlbundle/manifests/lvm.topolvm.io_lvmvolumegroups.yamlcatalog/lvms-operator/v0.0.1.yamlcmd/operator/operator.gocmd/vgmanager/vgmanager.goconfig/crd/bases/lvm.topolvm.io_lvmclusters.yamlconfig/crd/bases/lvm.topolvm.io_lvmvolumegroupnodestatuses.yamlconfig/crd/bases/lvm.topolvm.io_lvmvolumegroups.yamlgo.modinternal/controllers/vgmanager/validatelvs_test.go
✅ Files skipped from review due to trivial changes (2)
- bundle/manifests/lvm.topolvm.io_lvmclusters.yaml
- config/crd/bases/lvm.topolvm.io_lvmclusters.yaml
|
@qJkee: This pull request references OCPEDGE-2346 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 story to target the "4.22.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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/operator/operator.go (1)
244-257: Consider usingcontext.WithCancelCausefor consistency with vgmanager.Operator uses plain
cancel()while vgmanager usescancelWithCause(ErrTLSProfileModified). The operator's exit path at line 381 returns nil without distinguishing the shutdown reason. This works but makes debugging harder—logs won't indicate the restart was due to TLS profile changes.If the current behavior is intentional (relying on container restart), this is acceptable. Otherwise, consider aligning with vgmanager's pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/operator/operator.go` around lines 244 - 257, The OnProfileChange handler in SecurityProfileWatcher currently calls cancel(), losing the shutdown cause; replace the plain context cancellation with context.WithCancelCause (or your project's cancelWithCause helper) and invoke cancelWithCause(ErrTLSProfileModified) (or equivalent) inside the OnProfileChange closure so the operator’s shutdown reason is preserved and can be checked/returned from the main run loop (adjust the function that receives the context so it surfaces the cancellation cause instead of just returning nil). Ensure references: SecurityProfileWatcher, OnProfileChange, cancel -> cancelWithCause/ErrTLSProfileModified so callers can log or return the TLS-profile-modified cause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/operator/operator.go`:
- Around line 244-257: The OnProfileChange handler in SecurityProfileWatcher
currently calls cancel(), losing the shutdown cause; replace the plain context
cancellation with context.WithCancelCause (or your project's cancelWithCause
helper) and invoke cancelWithCause(ErrTLSProfileModified) (or equivalent) inside
the OnProfileChange closure so the operator’s shutdown reason is preserved and
can be checked/returned from the main run loop (adjust the function that
receives the context so it surfaces the cancellation cause instead of just
returning nil). Ensure references: SecurityProfileWatcher, OnProfileChange,
cancel -> cancelWithCause/ErrTLSProfileModified so callers can log or return the
TLS-profile-modified cause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9c2daa23-9f75-4ad9-a71a-d7511a416972
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
bundle/manifests/lvm.topolvm.io_lvmclusters.yamlbundle/manifests/lvm.topolvm.io_lvmvolumegroupnodestatuses.yamlbundle/manifests/lvm.topolvm.io_lvmvolumegroups.yamlcatalog/lvms-operator/v0.0.1.yamlcmd/operator/operator.gocmd/vgmanager/vgmanager.goconfig/crd/bases/lvm.topolvm.io_lvmclusters.yamlconfig/crd/bases/lvm.topolvm.io_lvmvolumegroupnodestatuses.yamlconfig/crd/bases/lvm.topolvm.io_lvmvolumegroups.yamlgo.modinternal/controllers/vgmanager/validatelvs_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controllers/vgmanager/validatelvs_test.go
- bundle/manifests/lvm.topolvm.io_lvmvolumegroups.yaml
- config/crd/bases/lvm.topolvm.io_lvmclusters.yaml
- bundle/manifests/lvm.topolvm.io_lvmvolumegroupnodestatuses.yaml
|
/retest |
|
/lgtm |
|
/retest |
Read TLS config from API server and use it where applicable. When config changes - trigger graceful shutdown to recreate servers with new TLS configuration
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest |
|
@qJkee: 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. |
|
/lgtm |
5 participants
Read TLS config from API server and use it where applicable. When config changes - trigger graceful shutdown to recreate servers with new TLS configuration
Summary by CodeRabbit
New Features
Chores
Dependencies
Tests