wip: Trust Manager Controller Framework#371
Open
chiragkyal wants to merge 14 commits intoopenshift:masterfrom
Open
wip: Trust Manager Controller Framework#371chiragkyal wants to merge 14 commits intoopenshift:masterfrom
chiragkyal wants to merge 14 commits intoopenshift:masterfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-merge-robot
added
the
needs-rebase
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a TrustManager feature: new TrustManager and Bundle CRDs, controller implementation and reconciler, embedded trust-manager manifests/assets, generated client/informer/lister/applyconfig code, shared controller utilities and fakes, platform-aware optional-informer and gating, Makefile/hack changes, and unit + e2e tests. Changes
Sequence DiagramsequenceDiagram
participant User as Operator API
participant K8s as Kubernetes API
participant Controller as TrustManager Reconciler
participant Assets as Embedded Assets
participant Common as Shared Utilities
User->>K8s: create TrustManager (metadata.name=cluster)
K8s->>Controller: watch event / enqueue request
Controller->>K8s: Get TrustManager
K8s-->>Controller: TrustManager object
Controller->>Common: validate singleton / config / namespace
alt valid
Controller->>Assets: decode serviceAccount asset
Controller->>K8s: Server-side Apply ServiceAccount
K8s-->>Controller: ServiceAccount created/updated
Controller->>K8s: Update TrustManager status (observed state, conditions)
K8s-->>Controller: Status updated
Controller->>K8s: add annotation / finalizer
K8s-->>Controller: persisted
else invalid
Controller->>K8s: Update status Degraded / emit event
K8s-->>Controller: Status updated
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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: chiragkyal 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controller/common/errors.go (2)
82-90:⚠️ Potential issue | 🟡 MinorPreserve MultipleInstanceError when wrapping in FromError.
Currently, a
MultipleInstanceErrorpassed intoFromErroris reclassified asRetryRequired, which drops the reason.🧭 Suggested handling
func FromError(err error, message string, args ...any) *ReconcileError { if err == nil { return nil } + if IsMultipleInstanceError(err) { + return &ReconcileError{ + Reason: MultipleInstanceError, + Message: fmt.Sprintf(message, args...), + Err: err, + } + } if IsIrrecoverableError(err) { return NewIrrecoverableError(err, message, args...) } return NewRetryRequiredError(err, message, args...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/common/errors.go` around lines 82 - 90, FromError currently rewraps any non-nil error into NewRetryRequiredError unless IsIrrecoverableError holds, which causes a MultipleInstanceError to lose its specific type/reason; update FromError to detect if err is a *MultipleInstanceError (or satisfies an IsMultipleInstanceError predicate) and return that exact error (or wrap preserving its MultipleInstanceError status) instead of converting it to NewRetryRequiredError; keep existing checks for IsIrrecoverableError and use NewIrrecoverableError for those cases, but ensure MultipleInstanceError is returned/preserved by FromError so callers receive the original reason.
69-79:⚠️ Potential issue | 🟠 MajorRemove
apierrors.IsServiceUnavailablefrom the irrecoverable error condition.Kubernetes controller-runtime guidance explicitly treats HTTP 503 (Service Unavailable) as a transient error that should trigger requeues with exponential backoff. Marking it irrecoverable suppresses requeues during transient API server outages, breaking controller resilience across 40+ usage sites in the codebase.
🔧 Proposed fix
- if apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err) || apierrors.IsInvalid(err) || - apierrors.IsBadRequest(err) || apierrors.IsServiceUnavailable(err) { + if apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err) || apierrors.IsInvalid(err) || + apierrors.IsBadRequest(err) { return NewIrrecoverableError(err, message, args...) } return NewRetryRequiredError(err, message, args...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/common/errors.go` around lines 69 - 79, The current FromClientError function treats apierrors.IsServiceUnavailable as irrecoverable; remove apierrors.IsServiceUnavailable from the conditional that returns NewIrrecoverableError so that 503 Service Unavailable falls through to the retry path and returns NewRetryRequiredError instead; update the condition around apierrors.IsUnauthorized, apierrors.IsForbidden, apierrors.IsInvalid, and apierrors.IsBadRequest in FromClientError accordingly to exclude IsServiceUnavailable.
🧹 Nitpick comments (11)
pkg/operator/platformutil/optional_informer_test.go (2)
22-30: Consider adding descriptive names to test cases.Adding a
namefield to the test cases improves test output readability and makes it easier to identify which case failed.♻️ Suggested improvement
tests := []struct { + name string isCRDPresent bool expectInformer bool }{ - // positive cases with no error - // false => false, true => true - {isCRDPresent: false, expectInformer: false}, - {isCRDPresent: true, expectInformer: true}, + {name: "CRD absent - informer not applicable", isCRDPresent: false, expectInformer: false}, + {name: "CRD present - informer applicable", isCRDPresent: true, expectInformer: true}, } for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { fakeClient := createFakeClient(tt.isCRDPresent) // ... rest of test + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/platformutil/optional_informer_test.go` around lines 22 - 30, Add a descriptive `name` field to the test case struct in optional_informer_test.go (the tests slice that currently has isCRDPresent and expectInformer) and use t.Run with that name for each subtest; update the loop that iterates the tests to call t.Run(tc.name, func(t *testing.T) { ... }) so failures are reported with the case name and make it easier to identify which {isCRDPresent, expectInformer} case failed.
39-41: Calling unexporteddiscover()method in test.The test calls
optInformer.discover()which is an unexported method. This works because the test is in the same package, but it tests internal implementation details rather than the public API. Consider whether testing only through public methods (Applicable(),InformerFactory) would be sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/platformutil/optional_informer_test.go` around lines 39 - 41, The test is calling the unexported method discover() on optInformer which tests internal implementation; change the test to exercise the public API instead by asserting behavior via Applicable() and/or the InformerFactory exposure on the same optInformer instance (or by using the public constructor/initializer used by production code) so the test verifies whether CRDs are detected and informers created without invoking discover() directly; refactor the test to remove optInformer.discover() calls and replace them with assertions against optInformer.Applicable() and checks on optInformer.InformerFactory (or other public outputs) to validate CRD presence.pkg/controller/common/utils.go (1)
19-31: Clarify that HasObjectChanged only compares labels.The name/comment reads like a full object diff, but only labels are checked. Consider updating the comment (or name) to prevent misuse.
✏️ Suggested comment tweak
-// HasObjectChanged compares two objects of the same type and returns true if they differ. -// Returns false if the objects are not of the same type. +// HasObjectChanged compares label metadata of two objects of the same type. +// Returns false if the objects are not of the same type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/common/utils.go` around lines 19 - 31, The function HasObjectChanged (which calls ObjectMetadataModified and uses desired.GetLabels() / fetched.GetLabels()) only compares object labels, but its name and comment imply a full object diff; update the comment and/or rename the API to avoid misuse—either change the HasObjectChanged comment to explicitly state it only compares labels (e.g., "HasObjectChanged compares only labels of two objects and returns true if labels differ") or rename HasObjectChanged to HasLabelsChanged and adjust comments accordingly, leaving ObjectMetadataModified's comment to state it compares labels via reflect.DeepEqual(desired.GetLabels(), fetched.GetLabels()).config/crd/kustomization.yaml (1)
13-14: Consider using consistent file extensions.The Bundle CRD file uses
.ymlextension (customresourcedefinition_bundles.trust.cert-manager.io.yml) while all other CRD files in this list use.yaml. This might be intentional if the file is imported from upstream trust-manager, but for consistency within this repository, consider renaming to use.yaml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/crd/kustomization.yaml` around lines 13 - 14, The kustomization lists "customresourcedefinition_bundles.trust.cert-manager.io.yml" with a .yml extension that is inconsistent with other CRD entries; rename the file to "customresourcedefinition_bundles.trust.cert-manager.io.yaml" in the repo and update the reference in config/crd/kustomization.yaml (replace the .yml entry) so all CRD filenames use the .yaml extension and any other references to customresourcedefinition_bundles.trust.cert-manager.io.yml are updated accordingly.hack/update-trust-manager-manifests.sh (1)
36-47: Delay deletion until after a successful split to avoid losing artifacts on failure.If the render/split step fails, the current order leaves the repo missing generated resources. Consider moving the cleanup after
yq -scompletes successfully.♻️ Suggested reordering
-# regenerate all bindata -rm -rf bindata/trust-manager/resources -rm -f config/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml - -# split into individual manifest files -yq '... comments=""' -s '"_output/manifests/" + .kind + "_" + .metadata.name + ".yml" | downcase' ${MANIFESTS_PATH}/manifests.yaml - -# Move resource manifests to appropriate location +# split into individual manifest files +yq '... comments=""' -s '"_output/manifests/" + .kind + "_" + .metadata.name + ".yml" | downcase' ${MANIFESTS_PATH}/manifests.yaml + +# regenerate all bindata (after successful split) +rm -rf bindata/trust-manager/resources +rm -f config/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml + +# Move resource manifests to appropriate location mkdir -p bindata/trust-manager/resources🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/update-trust-manager-manifests.sh` around lines 36 - 47, The cleanup (rm -rf bindata/trust-manager/resources and rm -f config/crd/bases/...) currently runs before the yq split and can delete generated assets on failure—change the script so the yq '... comments=""' -s split (using ${MANIFESTS_PATH}/manifests.yaml) runs first and only upon success perform the removals and the mkdir/mv steps; in practice, run the yq step, verify it exited successfully (or use an error-check/guard like checking the command status), then execute the cleanup and the moves (mv ${MANIFESTS_PATH}/customresourcedefinition_* and mv ${MANIFESTS_PATH}/*.yml into bindata/trust-manager/resources and config/crd/bases/); ensure no deletions happen before the split completes successfully.pkg/operator/platformutil/optional_informer.go (1)
22-27: Remove unusedctxparameter or rename to_.Go's standard compiler allows unused parameters without error, but this is poor practice. Clean it up by either removing it from the signature or renaming it to
_if the signature must match an interface.Fix (keep signature)
func NewOptionalInformer[groupInformer any]( - ctx context.Context, + _ context.Context, gvr schema.GroupVersionResource, discoveryClient discovery.DiscoveryInterface, informerInitFunc func() groupInformer, ) (*OptionalInformer[groupInformer], error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/platformutil/optional_informer.go` around lines 22 - 27, The NewOptionalInformer function currently declares an unused parameter ctx; remove the unused parameter from the function signature (i.e., drop ctx) and update any callers to stop passing it, or if the signature must remain for compatibility, rename the parameter to _ (i.e., use _ context.Context) to signal intentional unusedness; locate NewOptionalInformer and adjust its signature and all call sites or the implementing interface accordingly so there are no unused-parameter warnings.pkg/operator/platformutil/testing_helpers_test.go (1)
62-68: Variable shadowing:fakeshadows the imported package.The local variable
fakeshadows the importedfakepackage from line 12. While this works, it can cause confusion.Proposed fix
// createFakeDiscoveryWithResources creates a fake discovery client with the specified API resources. // Pass nil or empty slice for a discovery client with no resources. func createFakeDiscoveryWithResources(resources []*metav1.APIResourceList) *fakediscovery.FakeDiscovery { - fake := &fakediscovery.FakeDiscovery{Fake: &clienttesting.Fake{}} - fake.Resources = resources - return fake + fakeDiscovery := &fakediscovery.FakeDiscovery{Fake: &clienttesting.Fake{}} + fakeDiscovery.Resources = resources + return fakeDiscovery }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/platformutil/testing_helpers_test.go` around lines 62 - 68, The local variable named `fake` in function createFakeDiscoveryWithResources shadows the imported `fake` package; rename the local variable (e.g., to fakeDiscovery, fd, or fakeClient) and update its usages inside createFakeDiscoveryWithResources to avoid shadowing the imported package and eliminate confusion while keeping the behavior of fakediscovery.FakeDiscovery instantiation (`&fakediscovery.Fake{Fake: &clienttesting.Fake{}}`) unchanged.pkg/controller/trustmanager/controller.go (3)
167-167: Redundant nil initialization.
var errUpdate error = nilcan be simplified tovar errUpdate errorsince the zero value forerroris alreadynil.✨ Suggested fix
- var errUpdate error = nil + var errUpdate error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/controller.go` at line 167, The declaration `var errUpdate error = nil` is redundant because the zero value of an error is already nil; update the declaration for the `errUpdate` variable in controller.go (symbol: errUpdate) to simply `var errUpdate error` to remove the explicit `= nil`, leaving behavior unchanged and satisfying the style suggestion.
56-62: Avoid storingcontext.Background()in struct fields.Storing a context in the struct is an anti-pattern. The context should flow from the caller (the
Reconcilemethod receivesctxas a parameter). The storedr.ctxis used inprocessReconcileRequestand other methods, but this loses the context lifecycle benefits (cancellation, timeouts, tracing).Consider passing
ctxexplicitly to methods that need it instead of storing it in the struct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/controller.go` around lines 56 - 62, The Reconciler currently stores context.Background() in its ctx field (Reconciler.ctx), which is an anti-pattern; remove the ctx field from the Reconciler struct, stop initializing it in the constructor, and update all methods (e.g., processReconcileRequest and any helper methods) to accept a context parameter passed from the Reconcile(ctx, req) method so the caller-provided context (with cancellation/timeouts/tracing) flows through instead of a global background context.
174-175: Inconsistent reason constant for Ready=False condition.Using
ReasonReadywhen setting theReadycondition toFalseis semantically confusing. Consider using a more appropriate reason likeReasonFailedor introducing a dedicated reason constant for degraded states.✨ Suggested fix
degradedChanged := trustManager.Status.SetCondition(v1alpha1.Degraded, metav1.ConditionTrue, v1alpha1.ReasonFailed, fmt.Sprintf("reconciliation failed with irrecoverable error not retrying: %v", err)) - readyChanged := trustManager.Status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, v1alpha1.ReasonReady, "") + readyChanged := trustManager.Status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, v1alpha1.ReasonFailed, "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/controller.go` around lines 174 - 175, The Ready=False condition is incorrectly using v1alpha1.ReasonReady; update the call to trustManager.Status.SetCondition so the Ready condition uses a semantically appropriate reason (e.g., v1alpha1.ReasonFailed or a new constant like v1alpha1.ReasonDegraded) instead of v1alpha1.ReasonReady; adjust the second line that calls trustManager.Status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, ...) to pass the chosen reason constant and ensure any new constant is declared in the v1alpha1 package if you introduce ReasonDegraded.pkg/controller/trustmanager/utils.go (1)
113-119: Panic on decode failure may be acceptable but consider error handling.
decodeServiceAccountObjBytespanics if decoding fails. While this is acceptable for embedded assets that must be valid at compile time, consider if a more graceful error handling approach would be appropriate. If the assets are truly baked in and guaranteed valid, a comment explaining this would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/utils.go` around lines 113 - 119, The function decodeServiceAccountObjBytes currently panics on decode failure (decodeServiceAccountObjBytes using runtime.Decode and codecs.UniversalDecoder returning obj.(*corev1.ServiceAccount)); either change the signature to return (*corev1.ServiceAccount, error) and propagate the decode error instead of panicking, or if the bytes are guaranteed baked-in assets, add a clear comment above decodeServiceAccountObjBytes stating that assets are compile-time guaranteed valid and that panic is intentional for programming errors; pick one approach and update callers or add the explanatory comment accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/operator/v1alpha1/features.go`:
- Around line 28-29: The FeatureTrustManager feature is currently set to
Default: true for testing; change its default to Default: false in features.go
so the Beta feature is opt-in by default, updating the FeatureTrustManager entry
(the map key named FeatureTrustManager) to use Default: false and leaving
PreRelease: featuregate.Beta unchanged.
In `@bundle/manifests/trust.cert-manager.io_bundles.yaml`:
- Around line 486-493: The CRD field name defaultCAVersion and its description
reference defaultCAPackageVersion, causing a naming mismatch; update the
description text to reference defaultCAVersion (or alternatively rename the
field to defaultCAPackageVersion if that is the intended canonical name) so the
name and description match, and ensure references in the same object/type and
any related documentation or comments (the schema block containing
defaultCAVersion) are updated consistently.
In `@pkg/controller/istiocsr/deployments.go`:
- Around line 239-241: The error message in the issuer kind validation uses
issuerKind twice so the "configured" value is wrong; update the
NewIrrecoverableError call in the issuer kind check to pass the actual
configured value (issuerRefKind) as the last format argument instead of
issuerKind so the message shows the configured kind; locate the check that
references issuerRefKind, clusterIssuerKind, issuerKind and
invalidIssuerRefConfigError and replace the incorrect format argument.
In `@pkg/controller/trustmanager/install_trustmanager.go`:
- Around line 73-80: The code dereferences optional blocks
trustManager.Spec.TrustManagerConfig.SecretTargets and .DefaultCAPackage
directly which can cause nil-pointer panics; update the reconciliation logic
around the comparisons that set trustManager.Status.SecretTargetsPolicy and
trustManager.Status.DefaultCAPackagePolicy to first check that
trustManager.Spec.TrustManagerConfig is non-nil and that SecretTargets and
DefaultCAPackage are non-nil (and that their Policy fields are present or have
safe zero-values) before reading .Policy, and only set the corresponding status
fields and changed flag when those blocks exist (or explicitly handle the absent
case, e.g., clear the status field) so the code in the SecretTargetsPolicy and
DefaultCAPackagePolicy update paths is safe.
In `@pkg/controller/trustmanager/utils.go`:
- Around line 128-137: The aggregation incorrectly passes the status update
error twice, losing the original prependErr; in Reconciler.updateCondition
(which calls updateStatus) change the aggregate construction so it includes the
original prependErr and the constructed errUpdate (e.g.
utilerrors.NewAggregate([]error{prependErr, errUpdate})), ensuring prependErr is
non-nil when building the slice, and return that aggregate instead of repeating
err.
In `@pkg/operator/setup_manager.go`:
- Around line 188-221: In addControllerCacheConfig, the current map lookup uses
client.Object pointer identity so identical types across different instances
won't match; instead, find existing entries by comparing types (e.g., use
resType := fmt.Sprintf("%T", res) and loop the keys of objectList comparing
fmt.Sprintf("%T", k) to resType), capture the matching map key (call it
existingKey), then merge/replace objectList[existingKey] with the combined label
selector (rather than objectList[res]); for new types continue to insert
objectList[res] as before—ensure you use the existingKey when logging and
creating the merged requirement so type-based matches correctly trigger the
merge path.
In `@pkg/operator/starter.go`:
- Around line 164-178: The unified controller manager is started with
ctrl.SetupSignalHandler() which ignores the caller's ctx; change the manager
startup to use the caller's context so it shuts down when ctx is cancelled.
Locate NewControllerManager/ControllerConfig where manager is created and the
goroutine that calls manager.Start(ctrl.SetupSignalHandler()); replace the
signal-handler context with the existing caller ctx (the same ctx used by other
controllers like controller.Run and informer.Start) so manager.Start receives
ctx and aligns its lifecycle with the rest of the function.
In `@test/e2e/trustmanager_test.go`:
- Around line 22-26: Rename the misspelled constant
trustManagerServiceAccoutName to trustManagerServiceAccountName and update every
usage to the new identifier in this file (references that create or reference
the service account, e.g., where trustManagerServiceAccoutName is passed to
functions or compared). Ensure the constant declaration (const block containing
trustManagerNamespace and trustManagerCommonName) is updated and replace all
occurrences throughout the test (including tests that construct or assert the
trust-manager service account) to use trustManagerServiceAccountName so the
symbol matches everywhere.
- Around line 44-54: The redundant stale error check is happening in the
BeforeAll block: you call kubernetes.NewForConfig(cfg) (assigning err) and then
later have a leftover Expect(err).NotTo(HaveOccurred()) intended for the
commented-out patchSubscriptionWithEnvVars call; either remove the trailing
Expect(err).NotTo(HaveOccurred()) or move that Expect inside the commented block
(next to patchSubscriptionWithEnvVars) and ensure any new call that sets err
(e.g., patchSubscriptionWithEnvVars(ctx, loader, ...)) is followed by its own
Expect(err).NotTo(HaveOccurred()); update the BeforeAll to only assert the error
returned by kubernetes.NewForConfig(cfg) immediately after that call.
---
Outside diff comments:
In `@pkg/controller/common/errors.go`:
- Around line 82-90: FromError currently rewraps any non-nil error into
NewRetryRequiredError unless IsIrrecoverableError holds, which causes a
MultipleInstanceError to lose its specific type/reason; update FromError to
detect if err is a *MultipleInstanceError (or satisfies an
IsMultipleInstanceError predicate) and return that exact error (or wrap
preserving its MultipleInstanceError status) instead of converting it to
NewRetryRequiredError; keep existing checks for IsIrrecoverableError and use
NewIrrecoverableError for those cases, but ensure MultipleInstanceError is
returned/preserved by FromError so callers receive the original reason.
- Around line 69-79: The current FromClientError function treats
apierrors.IsServiceUnavailable as irrecoverable; remove
apierrors.IsServiceUnavailable from the conditional that returns
NewIrrecoverableError so that 503 Service Unavailable falls through to the retry
path and returns NewRetryRequiredError instead; update the condition around
apierrors.IsUnauthorized, apierrors.IsForbidden, apierrors.IsInvalid, and
apierrors.IsBadRequest in FromClientError accordingly to exclude
IsServiceUnavailable.
---
Nitpick comments:
In `@config/crd/kustomization.yaml`:
- Around line 13-14: The kustomization lists
"customresourcedefinition_bundles.trust.cert-manager.io.yml" with a .yml
extension that is inconsistent with other CRD entries; rename the file to
"customresourcedefinition_bundles.trust.cert-manager.io.yaml" in the repo and
update the reference in config/crd/kustomization.yaml (replace the .yml entry)
so all CRD filenames use the .yaml extension and any other references to
customresourcedefinition_bundles.trust.cert-manager.io.yml are updated
accordingly.
In `@hack/update-trust-manager-manifests.sh`:
- Around line 36-47: The cleanup (rm -rf bindata/trust-manager/resources and rm
-f config/crd/bases/...) currently runs before the yq split and can delete
generated assets on failure—change the script so the yq '... comments=""' -s
split (using ${MANIFESTS_PATH}/manifests.yaml) runs first and only upon success
perform the removals and the mkdir/mv steps; in practice, run the yq step,
verify it exited successfully (or use an error-check/guard like checking the
command status), then execute the cleanup and the moves (mv
${MANIFESTS_PATH}/customresourcedefinition_* and mv ${MANIFESTS_PATH}/*.yml into
bindata/trust-manager/resources and config/crd/bases/); ensure no deletions
happen before the split completes successfully.
In `@pkg/controller/common/utils.go`:
- Around line 19-31: The function HasObjectChanged (which calls
ObjectMetadataModified and uses desired.GetLabels() / fetched.GetLabels()) only
compares object labels, but its name and comment imply a full object diff;
update the comment and/or rename the API to avoid misuse—either change the
HasObjectChanged comment to explicitly state it only compares labels (e.g.,
"HasObjectChanged compares only labels of two objects and returns true if labels
differ") or rename HasObjectChanged to HasLabelsChanged and adjust comments
accordingly, leaving ObjectMetadataModified's comment to state it compares
labels via reflect.DeepEqual(desired.GetLabels(), fetched.GetLabels()).
In `@pkg/controller/trustmanager/controller.go`:
- Line 167: The declaration `var errUpdate error = nil` is redundant because the
zero value of an error is already nil; update the declaration for the
`errUpdate` variable in controller.go (symbol: errUpdate) to simply `var
errUpdate error` to remove the explicit `= nil`, leaving behavior unchanged and
satisfying the style suggestion.
- Around line 56-62: The Reconciler currently stores context.Background() in its
ctx field (Reconciler.ctx), which is an anti-pattern; remove the ctx field from
the Reconciler struct, stop initializing it in the constructor, and update all
methods (e.g., processReconcileRequest and any helper methods) to accept a
context parameter passed from the Reconcile(ctx, req) method so the
caller-provided context (with cancellation/timeouts/tracing) flows through
instead of a global background context.
- Around line 174-175: The Ready=False condition is incorrectly using
v1alpha1.ReasonReady; update the call to trustManager.Status.SetCondition so the
Ready condition uses a semantically appropriate reason (e.g.,
v1alpha1.ReasonFailed or a new constant like v1alpha1.ReasonDegraded) instead of
v1alpha1.ReasonReady; adjust the second line that calls
trustManager.Status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, ...) to
pass the chosen reason constant and ensure any new constant is declared in the
v1alpha1 package if you introduce ReasonDegraded.
In `@pkg/controller/trustmanager/utils.go`:
- Around line 113-119: The function decodeServiceAccountObjBytes currently
panics on decode failure (decodeServiceAccountObjBytes using runtime.Decode and
codecs.UniversalDecoder returning obj.(*corev1.ServiceAccount)); either change
the signature to return (*corev1.ServiceAccount, error) and propagate the decode
error instead of panicking, or if the bytes are guaranteed baked-in assets, add
a clear comment above decodeServiceAccountObjBytes stating that assets are
compile-time guaranteed valid and that panic is intentional for programming
errors; pick one approach and update callers or add the explanatory comment
accordingly.
In `@pkg/operator/platformutil/optional_informer_test.go`:
- Around line 22-30: Add a descriptive `name` field to the test case struct in
optional_informer_test.go (the tests slice that currently has isCRDPresent and
expectInformer) and use t.Run with that name for each subtest; update the loop
that iterates the tests to call t.Run(tc.name, func(t *testing.T) { ... }) so
failures are reported with the case name and make it easier to identify which
{isCRDPresent, expectInformer} case failed.
- Around line 39-41: The test is calling the unexported method discover() on
optInformer which tests internal implementation; change the test to exercise the
public API instead by asserting behavior via Applicable() and/or the
InformerFactory exposure on the same optInformer instance (or by using the
public constructor/initializer used by production code) so the test verifies
whether CRDs are detected and informers created without invoking discover()
directly; refactor the test to remove optInformer.discover() calls and replace
them with assertions against optInformer.Applicable() and checks on
optInformer.InformerFactory (or other public outputs) to validate CRD presence.
In `@pkg/operator/platformutil/optional_informer.go`:
- Around line 22-27: The NewOptionalInformer function currently declares an
unused parameter ctx; remove the unused parameter from the function signature
(i.e., drop ctx) and update any callers to stop passing it, or if the signature
must remain for compatibility, rename the parameter to _ (i.e., use _
context.Context) to signal intentional unusedness; locate NewOptionalInformer
and adjust its signature and all call sites or the implementing interface
accordingly so there are no unused-parameter warnings.
In `@pkg/operator/platformutil/testing_helpers_test.go`:
- Around line 62-68: The local variable named `fake` in function
createFakeDiscoveryWithResources shadows the imported `fake` package; rename the
local variable (e.g., to fakeDiscovery, fd, or fakeClient) and update its usages
inside createFakeDiscoveryWithResources to avoid shadowing the imported package
and eliminate confusion while keeping the behavior of
fakediscovery.FakeDiscovery instantiation (`&fakediscovery.Fake{Fake:
&clienttesting.Fake{}}`) unchanged.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (106)
api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumtest/e2e/go.sumis excluded by!**/*.sumtools/go.sumis excluded by!**/*.sumvendor/github.com/maxbrunsfeld/counterfeiter/v6/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/maxbrunsfeld/counterfeiter/v6/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/maxbrunsfeld/counterfeiter/v6/arguments/parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/maxbrunsfeld/counterfeiter/v6/generator/fake.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/maxbrunsfeld/counterfeiter/v6/generator/interface_template.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/maxbrunsfeld/counterfeiter/v6/generator/loader.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/mod/modfile/read.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/mod/module/module.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/mod/semver/semver.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/diagnostic.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/appends/appends.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/asmdecl/asmdecl.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/assign/assign.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/atomic/atomic.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/bools/bools.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/buildssa/buildssa.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/buildtag/buildtag.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/cgocall/cgocall.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/copylock/copylock.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/ctrlflow/ctrlflow.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/deepequalerrors/deepequalerrors.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/defers/defers.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/directive/directive.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/errorsas/errorsas.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/fieldalignment/fieldalignment.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/framepointer/framepointer.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/httpmux/httpmux.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/ifaceassert/ifaceassert.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/inspect/inspect.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/internal/analysisutil/util.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/internal/ctrlflowinternal/ctrlflowinternal.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/loopclosure/loopclosure.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/lostcancel/lostcancel.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/nilfunc/nilfunc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/nilness/nilness.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/printf/printf.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/printf/types.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/reflectvaluecompare/reflectvaluecompare.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/shadow/shadow.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/sigchanyzer/sigchanyzer.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/slog/slog.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/stdmethods/stdmethods.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/stringintconv/string.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/testinggoroutine/testinggoroutine.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/testinggoroutine/util.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/tests/tests.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/timeformat/timeformat.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/unmarshal/unmarshal.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/unreachable/unreachable.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/unsafeptr/unsafeptr.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/unusedresult/unusedresult.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/unusedwrite/unusedwrite.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/passes/waitgroup/waitgroup.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ast/inspector/cursor.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/cfg/builder.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/cfg/cfg.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/packages/visit.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/builder.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/create.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/emit.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/func.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/instantiate.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/ssa.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/ssautil/visit.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/subst.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ssa/util.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/types/objectpath/objectpath.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/types/typeutil/map.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/imports/forward.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/doc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/extractdoc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/readfile.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/version.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/analysis/typeindex/typeindex.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/analysisinternal/analysis.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/astutil/stringlit.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/astutil/util.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/cfginternal/cfginternal.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/gcimporter/bimport.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/gcimporter/iexport.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/gcimporter/iimport.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/packagepath/packagepath.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/refactor/delete.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/refactor/imports.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/refactor/refactor.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/ssainternal/ssainternal.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/stdlib/deps.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/stdlib/import.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/stdlib/manifest.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typeparams/normalize.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/element.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/fx.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/isnamed.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/qualifier.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/varkind.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/varkind_go124.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/typesinternal/zerovalue.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/versions/features.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (97)
Makefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/meta.goapi/operator/v1alpha1/trustmanager_types.gobindata/trust-manager/resources/certificate_trust-manager.ymlbindata/trust-manager/resources/clusterrole_trust-manager.ymlbindata/trust-manager/resources/clusterrolebinding_trust-manager.ymlbindata/trust-manager/resources/deployment_trust-manager.ymlbindata/trust-manager/resources/issuer_trust-manager.ymlbindata/trust-manager/resources/role_trust-manager.ymlbindata/trust-manager/resources/role_trust-manager:leaderelection.ymlbindata/trust-manager/resources/rolebinding_trust-manager.ymlbindata/trust-manager/resources/rolebinding_trust-manager:leaderelection.ymlbindata/trust-manager/resources/service_trust-manager-metrics.ymlbindata/trust-manager/resources/service_trust-manager.ymlbindata/trust-manager/resources/serviceaccount_trust-manager.ymlbindata/trust-manager/resources/validatingwebhookconfiguration_trust-manager.ymlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_istiocsrs.yamlbundle/manifests/operator.openshift.io_trustmanagers.yamlbundle/manifests/trust.cert-manager.io_bundles.yamlconfig/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.ymlconfig/crd/bases/operator.openshift.io_istiocsrs.yamlconfig/crd/bases/operator.openshift.io_trustmanagers.yamlconfig/crd/kustomization.yamlconfig/manifests/bases/cert-manager-operator.clusterserviceversion.yamlconfig/rbac/role.yamlconfig/samples/tech-preview/kustomization.yamlconfig/samples/tech-preview/operator.openshift.io_v1alpha1_trustmanager.yamlconfig/samples/tech-preview/trust.cert-manager.io_v1alpha1_bundle.yamlgo.modhack/update-trust-manager-manifests.shpkg/controller/common/client.gopkg/controller/common/constants.gopkg/controller/common/errors.gopkg/controller/common/fakes/fake_ctrl_client.gopkg/controller/common/utils.gopkg/controller/deployment/cert_manager_cainjector_deployment.gopkg/controller/deployment/cert_manager_controller_deployment.gopkg/controller/deployment/cert_manager_controller_set.gopkg/controller/deployment/cert_manager_webhook_deployment.gopkg/controller/deployment/generic_deployment_controller.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/controller.gopkg/controller/istiocsr/controller_test.gopkg/controller/istiocsr/deployments.gopkg/controller/istiocsr/deployments_test.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/install_istiocsr.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services.gopkg/controller/istiocsr/services_test.gopkg/controller/istiocsr/utils.gopkg/controller/trustmanager/constants.gopkg/controller/trustmanager/controller.gopkg/controller/trustmanager/install_trustmanager.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/utils.gopkg/operator/applyconfigurations/internal/internal.gopkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagercontrollerconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.gopkg/operator/applyconfigurations/utils.gopkg/operator/assets/bindata.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_trustmanager.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/trustmanager.gopkg/operator/informers/externalversions/generic.gopkg/operator/informers/externalversions/operator/v1alpha1/interface.gopkg/operator/informers/externalversions/operator/v1alpha1/trustmanager.gopkg/operator/listers/operator/v1alpha1/expansion_generated.gopkg/operator/listers/operator/v1alpha1/trustmanager.gopkg/operator/optionalinformer/optional_informer.gopkg/operator/optionalinformer/optional_informer_test.gopkg/operator/platformutil/discovery.gopkg/operator/platformutil/discovery_test.gopkg/operator/platformutil/optional_informer.gopkg/operator/platformutil/optional_informer_test.gopkg/operator/platformutil/platform_checker.gopkg/operator/platformutil/platform_checker_test.gopkg/operator/platformutil/testing_helpers_test.gopkg/operator/setup_manager.gopkg/operator/starter.gotest/e2e/go.modtest/e2e/trustmanager_test.gotools/go.mod
💤 Files with no reviewable changes (2)
- pkg/operator/optionalinformer/optional_informer_test.go
- pkg/operator/optionalinformer/optional_informer.go
api/operator/v1alpha1/features.go
Outdated
Comment on lines
28
to
29
| // TODO: Change to false once testing is complete | ||
| FeatureTrustManager: {Default: true, PreRelease: featuregate.Beta}, |
There was a problem hiding this comment.
Reminder: Address TODO before merging to ensure feature is opt-in by default.
The TODO indicates that Default: true is temporary for testing. For a Beta feature, defaulting to false is typically safer to prevent unexpected behavior in production environments. Ensure this is changed to Default: false before the PR leaves WIP status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/operator/v1alpha1/features.go` around lines 28 - 29, The
FeatureTrustManager feature is currently set to Default: true for testing;
change its default to Default: false in features.go so the Beta feature is
opt-in by default, updating the FeatureTrustManager entry (the map key named
FeatureTrustManager) to use Default: false and leaving PreRelease:
featuregate.Beta unchanged.
Comment on lines
+486
to
+493
| defaultCAVersion: | ||
| description: |- | ||
| DefaultCAPackageVersion, if set and non-empty, indicates the version information | ||
| which was retrieved when the set of default CAs was requested in the bundle | ||
| source. This should only be set if useDefaultCAs was set to "true" on a source, | ||
| and will be the same for the same version of a bundle with identical certificates. | ||
| type: string | ||
| type: object |
There was a problem hiding this comment.
Field name vs description inconsistency.
The field is named defaultCAVersion but the description refers to defaultCAPackageVersion. This may cause confusion for users reading the CRD documentation.
Note: If this CRD is imported from upstream trust-manager, this should be reported upstream rather than fixed here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bundle/manifests/trust.cert-manager.io_bundles.yaml` around lines 486 - 493,
The CRD field name defaultCAVersion and its description reference
defaultCAPackageVersion, causing a naming mismatch; update the description text
to reference defaultCAVersion (or alternatively rename the field to
defaultCAPackageVersion if that is the intended canonical name) so the name and
description match, and ensure references in the same object/type and any related
documentation or comments (the schema block containing defaultCAVersion) are
updated consistently.
Comment on lines
239
to
241
| if issuerRefKind != clusterIssuerKind && issuerRefKind != issuerKind { | ||
| return NewIrrecoverableError(invalidIssuerRefConfigError, "spec.istioCSRConfig.certManager.issuerRef.kind can be anyof `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerKind) | ||
| return common.NewIrrecoverableError(invalidIssuerRefConfigError, "spec.istioCSRConfig.certManager.issuerRef.kind can be anyof `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerKind) | ||
| } |
There was a problem hiding this comment.
Bug: Error message shows issuerKind twice instead of the configured value.
The error message references issuerKind in both the "can be anyof" and "configured" parts, but should show the actual configured kind from the spec.
Proposed fix
- return common.NewIrrecoverableError(invalidIssuerRefConfigError, "spec.istioCSRConfig.certManager.issuerRef.kind can be anyof `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerKind)
+ return common.NewIrrecoverableError(invalidIssuerRefConfigError, "spec.istioCSRConfig.certManager.issuerRef.kind can be anyof `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerRefKind)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if issuerRefKind != clusterIssuerKind && issuerRefKind != issuerKind { | |
| return NewIrrecoverableError(invalidIssuerRefConfigError, "spec.istioCSRConfig.certManager.issuerRef.kind can be anyof `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerKind) | |
| return common.NewIrrecoverableError(invalidIssuerRefConfigError, "spec.istioCSRConfig.certManager.issuerRef.kind can be anyof `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerKind) | |
| } | |
| if issuerRefKind != clusterIssuerKind && issuerRefKind != issuerKind { | |
| return common.NewIrrecoverableError(invalidIssuerRefConfigError, "spec.istioCSRConfig.certManager.issuerRef.kind can be anyof `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerRefKind) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/istiocsr/deployments.go` around lines 239 - 241, The error
message in the issuer kind validation uses issuerKind twice so the "configured"
value is wrong; update the NewIrrecoverableError call in the issuer kind check
to pass the actual configured value (issuerRefKind) as the last format argument
instead of issuerKind so the message shows the configured kind; locate the check
that references issuerRefKind, clusterIssuerKind, issuerKind and
invalidIssuerRefConfigError and replace the incorrect format argument.
Comment on lines
+73
to
+80
| if policy := trustManager.Spec.TrustManagerConfig.SecretTargets.Policy; trustManager.Status.SecretTargetsPolicy != policy { | ||
| trustManager.Status.SecretTargetsPolicy = policy | ||
| changed = true | ||
| } | ||
|
|
||
| if policy := trustManager.Spec.TrustManagerConfig.DefaultCAPackage.Policy; trustManager.Status.DefaultCAPackagePolicy != policy { | ||
| trustManager.Status.DefaultCAPackagePolicy = policy | ||
| changed = true |
There was a problem hiding this comment.
Guard optional config blocks to avoid nil deref panic.
trustManager.Spec.TrustManagerConfig.SecretTargets and .DefaultCAPackage are optional in the CRD, but are dereferenced unconditionally. A minimal TrustManager (omitting these blocks) will panic during reconciliation.
🛠️ Suggested fix
- if policy := trustManager.Spec.TrustManagerConfig.SecretTargets.Policy; trustManager.Status.SecretTargetsPolicy != policy {
- trustManager.Status.SecretTargetsPolicy = policy
+ secretTargetsPolicy := v1alpha1.SecretTargetsPolicy("Disabled")
+ if trustManager.Spec.TrustManagerConfig.SecretTargets != nil {
+ secretTargetsPolicy = trustManager.Spec.TrustManagerConfig.SecretTargets.Policy
+ }
+ if trustManager.Status.SecretTargetsPolicy != secretTargetsPolicy {
+ trustManager.Status.SecretTargetsPolicy = secretTargetsPolicy
changed = true
}
- if policy := trustManager.Spec.TrustManagerConfig.DefaultCAPackage.Policy; trustManager.Status.DefaultCAPackagePolicy != policy {
- trustManager.Status.DefaultCAPackagePolicy = policy
+ defaultCAPackagePolicy := v1alpha1.DefaultCAPackagePolicy("Disabled")
+ if trustManager.Spec.TrustManagerConfig.DefaultCAPackage != nil {
+ defaultCAPackagePolicy = trustManager.Spec.TrustManagerConfig.DefaultCAPackage.Policy
+ }
+ if trustManager.Status.DefaultCAPackagePolicy != defaultCAPackagePolicy {
+ trustManager.Status.DefaultCAPackagePolicy = defaultCAPackagePolicy
changed = true
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/trustmanager/install_trustmanager.go` around lines 73 - 80,
The code dereferences optional blocks
trustManager.Spec.TrustManagerConfig.SecretTargets and .DefaultCAPackage
directly which can cause nil-pointer panics; update the reconciliation logic
around the comparisons that set trustManager.Status.SecretTargetsPolicy and
trustManager.Status.DefaultCAPackagePolicy to first check that
trustManager.Spec.TrustManagerConfig is non-nil and that SecretTargets and
DefaultCAPackage are non-nil (and that their Policy fields are present or have
safe zero-values) before reading .Policy, and only set the corresponding status
fields and changed flag when those blocks exist (or explicitly handle the absent
case, e.g., clear the status field) so the code in the SecretTargetsPolicy and
DefaultCAPackagePolicy update paths is safe.
Comment on lines
+128
to
+137
| func (r *Reconciler) updateCondition(trustManager *v1alpha1.TrustManager, prependErr error) error { | ||
| if err := r.updateStatus(r.ctx, trustManager); err != nil { | ||
| errUpdate := fmt.Errorf("failed to update %s status: %w", trustManager.GetName(), err) | ||
| if prependErr != nil { | ||
| return utilerrors.NewAggregate([]error{err, errUpdate}) | ||
| } | ||
| return errUpdate | ||
| } | ||
| return prependErr | ||
| } |
There was a problem hiding this comment.
Bug: Wrong error passed to aggregate.
In the error aggregation on line 132, err (the status update error) is passed twice instead of including prependErr. This means if both prependErr and the status update error exist, prependErr is lost.
🐛 Proposed fix
func (r *Reconciler) updateCondition(trustManager *v1alpha1.TrustManager, prependErr error) error {
if err := r.updateStatus(r.ctx, trustManager); err != nil {
errUpdate := fmt.Errorf("failed to update %s status: %w", trustManager.GetName(), err)
if prependErr != nil {
- return utilerrors.NewAggregate([]error{err, errUpdate})
+ return utilerrors.NewAggregate([]error{prependErr, errUpdate})
}
return errUpdate
}
return prependErr
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (r *Reconciler) updateCondition(trustManager *v1alpha1.TrustManager, prependErr error) error { | |
| if err := r.updateStatus(r.ctx, trustManager); err != nil { | |
| errUpdate := fmt.Errorf("failed to update %s status: %w", trustManager.GetName(), err) | |
| if prependErr != nil { | |
| return utilerrors.NewAggregate([]error{err, errUpdate}) | |
| } | |
| return errUpdate | |
| } | |
| return prependErr | |
| } | |
| func (r *Reconciler) updateCondition(trustManager *v1alpha1.TrustManager, prependErr error) error { | |
| if err := r.updateStatus(r.ctx, trustManager); err != nil { | |
| errUpdate := fmt.Errorf("failed to update %s status: %w", trustManager.GetName(), err) | |
| if prependErr != nil { | |
| return utilerrors.NewAggregate([]error{prependErr, errUpdate}) | |
| } | |
| return errUpdate | |
| } | |
| return prependErr | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/trustmanager/utils.go` around lines 128 - 137, The aggregation
incorrectly passes the status update error twice, losing the original
prependErr; in Reconciler.updateCondition (which calls updateStatus) change the
aggregate construction so it includes the original prependErr and the
constructed errUpdate (e.g. utilerrors.NewAggregate([]error{prependErr,
errUpdate})), ensuring prependErr is non-nil when building the slice, and return
that aggregate instead of repeating err.
Comment on lines
+188
to
+221
| func addControllerCacheConfig(objectList map[client.Object]cache.ByObject, labelValue string, resources []client.Object) error { | ||
| labelKey := common.ManagedResourceLabelKey | ||
|
|
||
| for _, res := range resources { | ||
| resType := fmt.Sprintf("%T", res) | ||
|
|
||
| if existing, exists := objectList[res]; exists { | ||
| // Resource already configured by another controller | ||
| // Merge label values using 'In' operator: app in (value1, value2) | ||
| existingReqs, _ := existing.Label.Requirements() | ||
| var existingValues []string | ||
| for _, req := range existingReqs { | ||
| if req.Key() == labelKey { | ||
| existingValues = req.Values().List() | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // Create new requirement with both values | ||
| mergedValues := append(existingValues, labelValue) | ||
| mergedReq, err := labels.NewRequirement(labelKey, selection.In, mergedValues) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create merged label requirement for key %q with values %v: %w", labelKey, mergedValues, err) | ||
| } | ||
| objectList[res] = cache.ByObject{Label: labels.NewSelector().Add(*mergedReq)} | ||
| setupLog.V(4).Info("merged label selector for shared resource", "type", resType, "values", mergedValues) | ||
| } else { | ||
| // First controller to configure this resource | ||
| labelReq, err := labels.NewRequirement(labelKey, selection.Equals, []string{labelValue}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create label requirement for key %q with value %q: %w", labelKey, labelValue, err) | ||
| } | ||
| objectList[res] = cache.ByObject{Label: labels.NewSelector().Add(*labelReq)} | ||
| } |
There was a problem hiding this comment.
Fix label-selector merge for shared resource types.
Line 194 uses a map key of client.Object, which compares pointer identity. If two controllers register the same kind (e.g., corev1.ServiceAccount in both lists), the merge path won’t trigger and selector choice becomes non-deterministic. Use type-based matching to find existing entries.
🛠️ Proposed fix (type-based lookup)
@@
-import (
+import (
"context"
"fmt"
+ "reflect"
@@
func addControllerCacheConfig(objectList map[client.Object]cache.ByObject, labelValue string, resources []client.Object) error {
labelKey := common.ManagedResourceLabelKey
for _, res := range resources {
- resType := fmt.Sprintf("%T", res)
+ resType := reflect.TypeOf(res)
+
+ var existingObj client.Object
+ var existing cache.ByObject
+ for obj, cfg := range objectList {
+ if reflect.TypeOf(obj) == resType {
+ existingObj = obj
+ existing = cfg
+ break
+ }
+ }
- if existing, exists := objectList[res]; exists {
+ if existingObj != nil {
// Resource already configured by another controller
// Merge label values using 'In' operator: app in (value1, value2)
existingReqs, _ := existing.Label.Requirements()
var existingValues []string
@@
- objectList[res] = cache.ByObject{Label: labels.NewSelector().Add(*mergedReq)}
- setupLog.V(4).Info("merged label selector for shared resource", "type", resType, "values", mergedValues)
+ objectList[existingObj] = cache.ByObject{Label: labels.NewSelector().Add(*mergedReq)}
+ setupLog.V(4).Info("merged label selector for shared resource", "type", resType.String(), "values", mergedValues)
} else {
// First controller to configure this resource
labelReq, err := labels.NewRequirement(labelKey, selection.Equals, []string{labelValue})
if err != nil {
return fmt.Errorf("failed to create label requirement for key %q with value %q: %w", labelKey, labelValue, err)
}
objectList[res] = cache.ByObject{Label: labels.NewSelector().Add(*labelReq)}
}
}
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/setup_manager.go` around lines 188 - 221, In
addControllerCacheConfig, the current map lookup uses client.Object pointer
identity so identical types across different instances won't match; instead,
find existing entries by comparing types (e.g., use resType := fmt.Sprintf("%T",
res) and loop the keys of objectList comparing fmt.Sprintf("%T", k) to resType),
capture the matching map key (call it existingKey), then merge/replace
objectList[existingKey] with the combined label selector (rather than
objectList[res]); for new types continue to insert objectList[res] as
before—ensure you use the existingKey when logging and creating the merged
requirement so type-based matches correctly trigger the merge path.
Comment on lines
+164
to
+178
| if istioCSREnabled || trustManagerEnabled { | ||
| // Create unified manager for all enabled operand controllers | ||
| manager, err := NewControllerManager(ControllerConfig{ | ||
| EnableIstioCSR: istioCSREnabled, | ||
| EnableTrustManager: trustManagerEnabled, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create unified controller manager: %w", err) | ||
| } | ||
|
|
||
| go func() { | ||
| if err := manager.Start(ctrl.SetupSignalHandler()); err != nil { | ||
| ctrl.Log.Error(err, "failed to start unified controller manager") | ||
| } | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd $(git rev-parse --root) && find . -name "starter.go" -type f | head -20Repository: openshift/cert-manager-operator
Length of output: 156
🏁 Script executed:
# Read the specific file and lines mentioned in the review
sed -n '160,185p' pkg/operator/starter.goRepository: openshift/cert-manager-operator
Length of output: 704
🏁 Script executed:
# Get more context around the code - find the RunOperator function
rg -n "func RunOperator" pkg/operator/starter.go -A 5Repository: openshift/cert-manager-operator
Length of output: 273
🏁 Script executed:
# Check the manager.Start() method signature
rg -n "func.*\(m.*Manager\).*Start" . -A 5 --type goRepository: openshift/cert-manager-operator
Length of output: 270
🏁 Script executed:
# Look for NewControllerManager definition
rg -n "func NewControllerManager" . --type go -A 15Repository: openshift/cert-manager-operator
Length of output: 1232
🏁 Script executed:
rg -n "SetupSignalHandler" pkg/operator/ --type go -B 2 -A 2Repository: openshift/cert-manager-operator
Length of output: 372
🏁 Script executed:
# Check if there's any cleanup or waiting for the goroutine
rg -n "sync\.WaitGroup|chan|sync\." pkg/operator/starter.go -A 2 -B 2Repository: openshift/cert-manager-operator
Length of output: 587
🏁 Script executed:
# Look for any test files that test RunOperator
find . -name "*starter*test*" -o -name "*test*starter*" | grep -E "\.go$"Repository: openshift/cert-manager-operator
Length of output: 57
🏁 Script executed:
# Check the full RunOperator function to see the complete flow
sed -n '46,200p' pkg/operator/starter.goRepository: openshift/cert-manager-operator
Length of output: 4759
Use the caller context for unified manager lifecycle.
The unified manager is started with ctrl.SetupSignalHandler(), which only responds to OS signals, while all other controllers in this function use the caller's ctx (e.g., controller.Run(ctx, 1), informer.Start(ctx.Done())). If ctx is cancelled without OS signals—such as in tests or during graceful shutdown—the manager goroutine continues running, creating a lifecycle mismatch. Pass ctx directly to align with the established pattern and ensure proper context-based shutdown.
🔧 Suggested change
go func() {
- if err := manager.Start(ctrl.SetupSignalHandler()); err != nil {
+ if err := manager.Start(ctx); err != nil {
ctrl.Log.Error(err, "failed to start unified controller manager")
}
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if istioCSREnabled || trustManagerEnabled { | |
| // Create unified manager for all enabled operand controllers | |
| manager, err := NewControllerManager(ControllerConfig{ | |
| EnableIstioCSR: istioCSREnabled, | |
| EnableTrustManager: trustManagerEnabled, | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to create unified controller manager: %w", err) | |
| } | |
| go func() { | |
| if err := manager.Start(ctrl.SetupSignalHandler()); err != nil { | |
| ctrl.Log.Error(err, "failed to start unified controller manager") | |
| } | |
| }() | |
| if istioCSREnabled || trustManagerEnabled { | |
| // Create unified manager for all enabled operand controllers | |
| manager, err := NewControllerManager(ControllerConfig{ | |
| EnableIstioCSR: istioCSREnabled, | |
| EnableTrustManager: trustManagerEnabled, | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to create unified controller manager: %w", err) | |
| } | |
| go func() { | |
| if err := manager.Start(ctx); err != nil { | |
| ctrl.Log.Error(err, "failed to start unified controller manager") | |
| } | |
| }() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/starter.go` around lines 164 - 178, The unified controller
manager is started with ctrl.SetupSignalHandler() which ignores the caller's
ctx; change the manager startup to use the caller's context so it shuts down
when ctx is cancelled. Locate NewControllerManager/ControllerConfig where
manager is created and the goroutine that calls
manager.Start(ctrl.SetupSignalHandler()); replace the signal-handler context
with the existing caller ctx (the same ctx used by other controllers like
controller.Run and informer.Start) so manager.Start receives ctx and aligns its
lifecycle with the rest of the function.
Comment on lines
+22
to
+26
| const ( | ||
| trustManagerNamespace = "cert-manager" | ||
| trustManagerServiceAccoutName = "trust-manager" | ||
| trustManagerCommonName = "cert-manager-trust-manager" | ||
| ) |
There was a problem hiding this comment.
Fix typo: trustManagerServiceAccoutName → trustManagerServiceAccountName
The constant name has a spelling error ("Accout" instead of "Account").
Proposed fix
const (
trustManagerNamespace = "cert-manager"
- trustManagerServiceAccoutName = "trust-manager"
+ trustManagerServiceAccountName = "trust-manager"
trustManagerCommonName = "cert-manager-trust-manager"
)Note: Update all usages at lines 92, 103, 109, and 114 as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const ( | |
| trustManagerNamespace = "cert-manager" | |
| trustManagerServiceAccoutName = "trust-manager" | |
| trustManagerCommonName = "cert-manager-trust-manager" | |
| ) | |
| const ( | |
| trustManagerNamespace = "cert-manager" | |
| trustManagerServiceAccountName = "trust-manager" | |
| trustManagerCommonName = "cert-manager-trust-manager" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/trustmanager_test.go` around lines 22 - 26, Rename the misspelled
constant trustManagerServiceAccoutName to trustManagerServiceAccountName and
update every usage to the new identifier in this file (references that create or
reference the service account, e.g., where trustManagerServiceAccoutName is
passed to functions or compared). Ensure the constant declaration (const block
containing trustManagerNamespace and trustManagerCommonName) is updated and
replace all occurrences throughout the test (including tests that construct or
assert the trust-manager service account) to use trustManagerServiceAccountName
so the symbol matches everywhere.
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
make update-dep PKG=github.com/maxbrunsfeld/counterfeiter/v6@latest Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
chiragkyal
force-pushed
the
tm-basic-controller
branch
from
6015556 to
49c167b
Compare
openshift-merge-robot
removed
the
needs-rebase
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
chiragkyal
force-pushed
the
tm-basic-controller
branch
from
ccdb4e5 to
5a4a234
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/common/errors.go (1)
82-91:⚠️ Potential issue | 🟠 MajorPreserve MultipleInstanceError in
FromError.
FromErrorcurrently rewraps aMultipleInstanceErrorasRetryRequiredError, which can cause unnecessary requeues for singleton violations. Add an explicit branch to keep the reason.✅ Proposed fix
func FromError(err error, message string, args ...any) *ReconcileError { if err == nil { return nil } if IsIrrecoverableError(err) { return NewIrrecoverableError(err, message, args...) } + if IsMultipleInstanceError(err) { + return &ReconcileError{ + Reason: MultipleInstanceError, + Message: fmt.Sprintf(message, args...), + Err: err, + } + } return NewRetryRequiredError(err, message, args...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/common/errors.go` around lines 82 - 91, FromError currently rewraps a MultipleInstanceError into a RetryRequiredError; change FromError to detect and preserve an existing MultipleInstanceError by returning it unchanged: inside FromError, before calling IsIrrecoverableError, check whether err is already a *ReconcileError (type assertion) and that its Reason equals MultipleInstanceError (or use a helper IsMultipleInstanceError if available); if so return the original *ReconcileError; otherwise continue with the existing IsIrrecoverableError -> NewIrrecoverableError / NewRetryRequiredError flow (functions: FromError, NewIrrecoverableError, NewRetryRequiredError, MultipleInstanceError, IsIrrecoverableError).
♻️ Duplicate comments (8)
pkg/operator/starter.go (1)
173-177:⚠️ Potential issue | 🟠 MajorLifecycle mismatch: unified manager uses signal handler instead of caller context.
The unified manager is started with
ctrl.SetupSignalHandler(), while all other controllers in this function use the caller'sctx. Ifctxis cancelled without OS signals (e.g., in tests or during graceful shutdown), the manager goroutine continues running.🛠️ Proposed fix
go func() { - if err := manager.Start(ctrl.SetupSignalHandler()); err != nil { + if err := manager.Start(ctx); err != nil { ctrl.Log.Error(err, "failed to start unified controller manager") } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/starter.go` around lines 173 - 177, The unified controller manager is started with ctrl.SetupSignalHandler() which ignores the caller's ctx; change the goroutine to call manager.Start with the provided ctx so the manager stops when ctx is cancelled (replace the ctrl.SetupSignalHandler() argument to manager.Start with the caller's ctx used by the other controllers, ensuring the error handling around manager.Start remains the same).pkg/operator/setup_manager.go (1)
188-224:⚠️ Potential issue | 🟠 MajorMap key comparison uses pointer identity - merge logic won't work for shared types.
The map lookup at line 194 (
if existing, exists := objectList[res]; exists) comparesclient.Objectby pointer identity. WhenistioCSRManagedResourcesandtrustManagerManagedResourcesboth contain&corev1.ServiceAccount{}, these are different pointer instances, so the merge path never triggers. The cache will contain duplicate entries with different selectors instead of a merged "In" selector.Use type-based comparison to find existing entries:
🛠️ Proposed fix using type-based lookup
func addControllerCacheConfig(objectList map[client.Object]cache.ByObject, labelValue string, resources []client.Object) error { labelKey := common.ManagedResourceLabelKey for _, res := range resources { resType := fmt.Sprintf("%T", res) - if existing, exists := objectList[res]; exists { + // Find existing entry by type, not pointer identity + var existingKey client.Object + var existing cache.ByObject + var exists bool + for k, v := range objectList { + if fmt.Sprintf("%T", k) == resType { + existingKey = k + existing = v + exists = true + break + } + } + + if exists { // Resource already configured by another controller // Merge label values using 'In' operator: app in (value1, value2) existingReqs, _ := existing.Label.Requirements() @@ // ... merging logic ... - objectList[res] = cache.ByObject{Label: labels.NewSelector().Add(*mergedReq)} + objectList[existingKey] = cache.ByObject{Label: labels.NewSelector().Add(*mergedReq)} setupLog.V(4).Info("merged label selector for shared resource", "type", resType, "values", mergedValues) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/setup_manager.go` around lines 188 - 224, The current addControllerCacheConfig uses objectList[res] (pointer identity) so different instances of the same resource type won't match; instead, search objectList for an existing entry by resource type (compare fmt.Sprintf("%T", key) or reflect.TypeOf(key).String() to resType) before deciding branch. Replace the direct map lookup if existing, exists := objectList[res] with a small loop over objectList keys to find a key whose type string equals resType (capture that map key as foundKey), then use objectList[foundKey] as existing and update objectList[foundKey] (or delete + reinsert using foundKey) when merging selectors; keep the rest of the merge logic (existingReqs, mergedValues, labels.NewRequirement, labels.NewSelector().Add) unchanged.test/e2e/trustmanager_test.go (2)
22-25:⚠️ Potential issue | 🟡 MinorFix typo in ServiceAccount constant name.
trustManagerServiceAccoutNameis misspelled and should betrustManagerServiceAccountNameto avoid confusion; update all usages.✏️ Suggested fix
- trustManagerServiceAccoutName = "trust-manager" + trustManagerServiceAccountName = "trust-manager"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/trustmanager_test.go` around lines 22 - 25, Rename the misspelled constant trustManagerServiceAccoutName to trustManagerServiceAccountName and update every reference to it; specifically modify the constant declaration in test/e2e/trustmanager_test.go (the const block that defines trustManagerNamespace, trustManagerServiceAccoutName, trustManagerCommonName) and replace all usages of trustManagerServiceAccoutName in tests and helpers (e.g., any test functions, setup helpers, or assertions referencing that symbol) to the corrected trustManagerServiceAccountName to keep identifiers consistent.
44-54:⚠️ Potential issue | 🟡 MinorRemove stale
Expect(err)after commented-out code.The trailing
Expect(err).NotTo(HaveOccurred())is now redundant and misleading becauseerrisn’t updated after the commented block.🧹 Suggested fix
- Expect(err).NotTo(HaveOccurred())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/trustmanager_test.go` around lines 44 - 54, The test's BeforeAll block contains a stale assertion "Expect(err).NotTo(HaveOccurred())" after the code that set err is commented out, which is redundant and misleading; remove the trailing Expect(err).NotTo(HaveOccurred()) or restore the call that sets err (e.g., re-enable the patchSubscriptionWithEnvVars call) so that the assertion validates the most recent operation—look for the BeforeAll function, the local variable err, and the commented patchSubscriptionWithEnvVars usage to decide whether to delete the extra Expect or reintroduce the operation that assigns err.pkg/controller/trustmanager/install_trustmanager.go (1)
73-80:⚠️ Potential issue | 🔴 CriticalGuard optional
SecretTargets/DefaultCAPackageto avoid nil deref.Both blocks are optional in the CRD, but their
Policyfields are dereferenced unconditionally, which can panic on minimal TrustManager CRs.🛡️ Suggested fix
- if policy := trustManager.Spec.TrustManagerConfig.SecretTargets.Policy; trustManager.Status.SecretTargetsPolicy != policy { - trustManager.Status.SecretTargetsPolicy = policy + secretTargetsPolicy := v1alpha1.SecretTargetsPolicy("Disabled") + if trustManager.Spec.TrustManagerConfig.SecretTargets != nil { + secretTargetsPolicy = trustManager.Spec.TrustManagerConfig.SecretTargets.Policy + } + if trustManager.Status.SecretTargetsPolicy != secretTargetsPolicy { + trustManager.Status.SecretTargetsPolicy = secretTargetsPolicy changed = true } - if policy := trustManager.Spec.TrustManagerConfig.DefaultCAPackage.Policy; trustManager.Status.DefaultCAPackagePolicy != policy { - trustManager.Status.DefaultCAPackagePolicy = policy + defaultCAPackagePolicy := v1alpha1.DefaultCAPackagePolicy("Disabled") + if trustManager.Spec.TrustManagerConfig.DefaultCAPackage != nil { + defaultCAPackagePolicy = trustManager.Spec.TrustManagerConfig.DefaultCAPackage.Policy + } + if trustManager.Status.DefaultCAPackagePolicy != defaultCAPackagePolicy { + trustManager.Status.DefaultCAPackagePolicy = defaultCAPackagePolicy changed = true }#!/bin/bash # Verify whether SecretTargets and DefaultCAPackage are pointer fields. rg -n --type=go -C3 'type TrustManagerConfig|SecretTargets|DefaultCAPackage' api/operator/v1alpha1/trustmanager_types.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/install_trustmanager.go` around lines 73 - 80, The code dereferences TrustManager.Spec.TrustManagerConfig.SecretTargets.Policy and DefaultCAPackage.Policy without nil checks causing panics; update the blocks in install_trustmanager.go to guard optional fields by first ensuring trustManager.Spec.TrustManagerConfig is non-nil and then checking that SecretTargets and DefaultCAPackage are non-nil before reading their Policy fields, e.g. only compare/set trustManager.Status.SecretTargetsPolicy when Spec.TrustManagerConfig.SecretTargets != nil and only compare/set trustManager.Status.DefaultCAPackagePolicy when Spec.TrustManagerConfig.DefaultCAPackage != nil (preserve setting changed = true when you update the status).config/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml (1)
463-468:⚠️ Potential issue | 🟡 MinorAlign
defaultCAVersiondescription with the field name.The description still references
DefaultCAPackageVersion, which doesn’t match the fielddefaultCAVersion. This is confusing for CRD consumers.✏️ Suggested fix
- defaultCAVersion: - description: |- - DefaultCAPackageVersion, if set and non-empty, indicates the version information + defaultCAVersion: + description: |- + DefaultCAVersion, if set and non-empty, indicates the version informationAfter updating the base CRD, please regenerate
bundle/manifests/trust.cert-manager.io_bundles.yamlto keep manifests in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml` around lines 463 - 468, Update the description string for the field defaultCAVersion so it references the actual field name (defaultCAVersion) instead of DefaultCAPackageVersion and clarifies semantics: state that defaultCAVersion holds the version identifier retrieved from the bundle source when useDefaultCAs is true and that it will be identical for bundles with the same certificates; then regenerate the bundle/manifests/trust.cert-manager.io_bundles.yaml to keep generated manifests in sync. Ensure you edit the description attached to the defaultCAVersion schema entry and leave references to useDefaultCAs and the bundle source intact.pkg/controller/istiocsr/deployments.go (1)
237-241:⚠️ Potential issue | 🟡 MinorShow the configured issuer kind in the error message.
The configured value in the message uses
issuerKindinstead of the actualissuerRefKind, which makes the error misleading.🐛 Suggested fix
- return common.NewIrrecoverableError(errInvalidIssuerRefConfig, "spec.istioCSRConfig.certManager.issuerRef.kind can be any of `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerKind) + return common.NewIrrecoverableError(errInvalidIssuerRefConfig, "spec.istioCSRConfig.certManager.issuerRef.kind can be any of `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerRefKind)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/deployments.go` around lines 237 - 241, The error message in Reconciler.assertIssuerRefExists uses issuerKind instead of the actual configured value; update the formatted NewIrrecoverableError call to pass issuerRefKind as the last argument so the message shows the real configured kind (symbols: assertIssuerRefExists, issuerRefKind, clusterIssuerKind, issuerKind, errInvalidIssuerRefConfig).pkg/controller/trustmanager/utils.go (1)
128-133:⚠️ Potential issue | 🟠 MajorAggregate loses the original error.
prependErris dropped and the status update error is duplicated.🐛 Fix aggregate to preserve the original error
- if prependErr != nil { - return utilerrors.NewAggregate([]error{err, errUpdate}) - } + if prependErr != nil { + return utilerrors.NewAggregate([]error{prependErr, errUpdate}) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/utils.go` around lines 128 - 133, In updateCondition, the original prependErr is being dropped and the status update error duplicated when building the aggregate; change the aggregate to include the original prependErr and the constructed errUpdate (not err twice). Locate the function Reconciler.updateCondition and replace the utilerrors.NewAggregate([]error{err, errUpdate}) call with utilerrors.NewAggregate([]error{prependErr, errUpdate}) (ensuring prependErr is non-nil in that branch) so the returned aggregate preserves the original error alongside the status update error.
🧹 Nitpick comments (11)
pkg/operator/platformutil/optional_informer.go (1)
22-27: Unusedctxparameter in constructor.The
ctxparameter is accepted but not used. Consider either:
- Removing it if discovery doesn't need context
- Passing it to
discover()ifResourceExistsshould support cancellation💡 Option 1: Remove unused parameter
func NewOptionalInformer[groupInformer any]( - ctx context.Context, gvr schema.GroupVersionResource, discoveryClient discovery.DiscoveryInterface, informerInitFunc func() groupInformer, ) (*OptionalInformer[groupInformer], error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/platformutil/optional_informer.go` around lines 22 - 27, The NewOptionalInformer constructor currently accepts a ctx parameter that is never used; either remove ctx from NewOptionalInformer’s signature and all call sites (if discovery doesn’t need cancellation), or thread ctx into the discovery call by passing it into OptionalInformer.discover/ResourceExists so discover can use ctx for cancellation; update the function signature and any invocations and modify discover/ResourceExists implementations to accept and use context.Context if you choose the second option (referencing NewOptionalInformer and OptionalInformer.discover/ResourceExists to locate the code).pkg/controller/common/client.go (1)
95-118: UpdateWithRetry doesn't check context cancellation between retries.The retry loop continues even if the context is cancelled. Consider checking
ctx.Done()within the retry function to respect cancellation and avoid unnecessary API calls.💡 Proposed fix to respect context cancellation
func (c *ctrlClientImpl) UpdateWithRetry( ctx context.Context, obj client.Object, opts ...client.UpdateOption, ) error { key := client.ObjectKeyFromObject(obj) if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } currentInterface := reflect.New(reflect.TypeOf(obj).Elem()).Interface() current, ok := currentInterface.(client.Object)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/common/client.go` around lines 95 - 118, The retry lambda in ctrlClientImpl.UpdateWithRetry uses retry.RetryOnConflict but never checks for context cancellation, so it keeps retrying after ctx is cancelled; modify the anonymous function passed to retry.RetryOnConflict (inside UpdateWithRetry) to check ctx.Done()/ctx.Err() at the start (or via a select) and return ctx.Err() (or context.Canceled) when cancelled so RetryOnConflict will stop making further attempts; ensure this check is performed before fetching the latest object and before each retry iteration.bindata/trust-manager/resources/deployment_trust-manager.yml (2)
40-45: Consider adding a liveness probe.The deployment has a readiness probe but no liveness probe. A liveness probe helps Kubernetes detect and restart containers that become stuck or unresponsive, improving overall reliability.
💡 Example liveness probe
readinessProbe: httpGet: port: 6060 path: /readyz initialDelaySeconds: 3 periodSeconds: 7 + livenessProbe: + httpGet: + port: 6060 + path: /readyz + initialDelaySeconds: 10 + periodSeconds: 10 + failureThreshold: 3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindata/trust-manager/resources/deployment_trust-manager.yml` around lines 40 - 45, Add a Kubernetes livenessProbe to the same container spec that currently has readinessProbe (look for readinessProbe, httpGet port: 6060, path: /readyz) so the container is restarted if it becomes unresponsive; use an httpGet to a suitable endpoint (e.g., /healthz or /readyz if appropriate) on port 6060, and configure reasonable initialDelaySeconds, periodSeconds and failureThreshold values to avoid false positives; ensure the new livenessProbe block mirrors the readinessProbe placement in the deployment_trust-manager.yml container spec.
66-66: Consider defining resource requests and limits.Empty resource constraints (
resources: {}) can lead to scheduling issues and resource contention in production. Consider specifying at least memory/CPU requests to ensure predictable scheduling and prevent resource starvation.💡 Example resource configuration
- resources: {} + resources: + requests: + cpu: 25m + memory: 32Mi + limits: + memory: 64Mi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindata/trust-manager/resources/deployment_trust-manager.yml` at line 66, The Deployment has an empty resources block ("resources: {}") which can cause scheduling and contention problems; update the container spec (spec.template.spec.containers[].resources) to include at minimum CPU and memory requests and ideally limits as well (e.g., set requests.cpu, requests.memory and limits.cpu, limits.memory) to ensure predictable scheduling and resource capping; choose values appropriate for trust-manager and document rationale in a comment or chart.pkg/controller/common/utils.go (3)
19-26: Semantic concern:HasObjectChangedreturns false for different types.Returning
falsewhen types differ seems counterintuitive - different types could be considered a significant change. However, this might be intentional to prevent comparing incompatible objects. Consider adding a comment explaining this design decision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/common/utils.go` around lines 19 - 26, Update the HasObjectChanged function comment to explain the deliberate choice to return false when desired and fetched are different concrete types rather than treating that as a change; explicitly state that type-mismatch is considered non-comparable and only metadata differences (via ObjectMetadataModified) are checked, so callers should handle type-mismatch (e.g., replacement) elsewhere. Reference the HasObjectChanged function and the ObjectMetadataModified helper in the comment so future readers understand the design intent.
33-37: Potential nil map access in ContainsAnnotation.If
obj.GetAnnotations()returnsnil, accessingobj.GetAnnotations()[annotation]will not panic (Go handles nil map reads gracefully), but it's worth noting for consistency withAddAnnotationwhich explicitly handles the nil case.💡 Optional: Add explicit nil check for consistency
func ContainsAnnotation(obj client.Object, annotation string) bool { + annotations := obj.GetAnnotations() + if annotations == nil { + return false + } - _, exist := obj.GetAnnotations()[annotation] + _, exist := annotations[annotation] return exist }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/common/utils.go` around lines 33 - 37, ContainsAnnotation currently reads obj.GetAnnotations()[annotation] without handling a nil map; update ContainsAnnotation to first retrieve the annotations map (e.g., anns := obj.GetAnnotations()), return false if anns == nil, then check the key and return the boolean. Reference the ContainsAnnotation function and mirror the nil-handling behavior used in AddAnnotation for consistency.
28-31: Function nameObjectMetadataModifiedis misleading.The function only compares labels, but the name suggests it compares all object metadata (annotations, finalizers, ownerReferences, etc.). Consider renaming to
LabelsModifiedor updating the implementation to match the name.♻️ Suggested rename
-// ObjectMetadataModified compares the labels of two objects and returns true if they differ. -func ObjectMetadataModified(desired, fetched client.Object) bool { +// LabelsModified compares the labels of two objects and returns true if they differ. +func LabelsModified(desired, fetched client.Object) bool { return !reflect.DeepEqual(desired.GetLabels(), fetched.GetLabels()) }Then update
HasObjectChangedto callLabelsModifiedinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/common/utils.go` around lines 28 - 31, The function ObjectMetadataModified only compares labels but its name implies all metadata; either rename it to LabelsModified and update all call sites (notably HasObjectChanged) to call LabelsModified, or expand ObjectMetadataModified to compare annotations, finalizers, ownerReferences, and labels (e.g., compare desired.GetLabels(), desired.GetAnnotations(), desired.GetFinalizers(), and desired.GetOwnerReferences() via reflect.DeepEqual against fetched). Choose one approach and apply it consistently: if renaming, replace ObjectMetadataModified with LabelsModified and update HasObjectChanged; if expanding, keep the name and add the extra field comparisons in the function.pkg/operator/setup_manager.go (1)
53-58: Reminder: Address the TODO for TrustManager managed resources.The list currently only contains
ServiceAccount. Based on the embedded assets inbindata.go, TrustManager also manages Deployment, ClusterRole, ClusterRoleBinding, Role, RoleBinding, Service, Certificate, Issuer, and ValidatingWebhookConfiguration resources.Would you like me to help generate the complete list of managed resources based on the trust-manager assets?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/setup_manager.go` around lines 53 - 58, The trustManagerManagedResources slice currently only lists ServiceAccount; update it to include all resource types managed by TrustManager per the embedded assets (Deployment, ClusterRole, ClusterRoleBinding, Role, RoleBinding, Service, Certificate, Issuer, ValidatingWebhookConfiguration) by adding the corresponding client.Object entries (e.g., &appsv1.Deployment{}, &rbacv1.ClusterRole{}, &rbacv1.ClusterRoleBinding{}, &rbacv1.Role{}, &rbacv1.RoleBinding{}, &corev1.Service{}, &certv1.Certificate{}, &certv1.Issuer{}, &admissionv1.ValidatingWebhookConfiguration{}) to the trustManagerManagedResources slice and update imports to include the matching API packages; remove or resolve the TODO once complete.pkg/controller/istiocsr/utils.go (1)
232-299: Confirm metadata comparison intentionally ignores annotations.
common.ObjectMetadataModifiedonly compares labels (pkg/controller/common/utils.go, Lines 28-30). If annotations are part of the desired state for any of these objects, changes won’t trigger updates. Consider extending the helper (or renaming it to clarify label-only semantics) to avoid missed reconciliation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/utils.go` around lines 232 - 299, The metadata comparison in hasObjectChanged uses common.ObjectMetadataModified which currently only compares labels, so annotation changes on resources (e.g., in hasObjectChanged for *certmanagerv1.Certificate, *appsv1.Deployment, *corev1.ConfigMap, etc.) will be ignored and not trigger reconciliation; update the metadata comparison to include annotations by either extending common.ObjectMetadataModified (add annotation comparison and rename if needed) or add a new helper like ObjectMetadataLabelsAndAnnotationsModified and call that from hasObjectChanged so annotation diffs are detected for all object types handled by hasObjectChanged.hack/update-trust-manager-manifests.sh (1)
40-48: Remove the combinedmanifests.yamlafter splitting.
Otherwise it gets moved into bindata alongside per-resource files, which can duplicate resources downstream.Suggested tweak
yq '... comments=""' -s '"_output/manifests/" + .kind + "_" + .metadata.name + ".yml" | downcase' ${MANIFESTS_PATH}/manifests.yaml +rm -f ${MANIFESTS_PATH}/manifests.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/update-trust-manager-manifests.sh` around lines 40 - 48, After splitting manifests with yq, remove the combined manifests file to avoid it being moved into bindata; update hack/update-trust-manager-manifests.sh to delete ${MANIFESTS_PATH}/manifests.yaml (or the appropriate combined file) after the yq split step and before the mv into bindata occurs so the subsequent mv ${MANIFESTS_PATH}/*.yml bindata/trust-manager/resources only moves per-resource files; reference the existing MANIFESTS_PATH variable and the yq split step to place the removal in the correct spot.pkg/controller/trustmanager/controller.go (1)
225-231: Cleanup TODO is clear; consider tracking it explicitly.If you want, I can open a follow‑up issue or draft the cleanup workflow when GA requirements are finalized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/controller.go` around lines 225 - 231, The cleanUp function currently has an untracked TODO about GA cleanup; create an explicit follow-up issue and reference it in the source so the work is discoverable: open a GitHub (or tracking) issue describing the cleanup workflow, then update the TODO comment inside the Reconciler.cleanUp function to include the issue number/URL and a short action item; also update the eventRecorder.Eventf call in cleanUp to include the tracking issue ID/URL and (if present) set a status condition on the TrustManager CR via the reconciler's status helper (e.g., set a "CleanupPending" or "CleanupNotImplemented" condition) so operators can see the backlog item from the object itself.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/istiocsr/deployments.go`:
- Around line 373-385: The type-assertion failures for issuerRefKind handling
(issuerRefKind, clusterIssuerKind, obj) are programming/data issues and should
be treated as irrecoverable; replace the current returns that call
common.FromClientError(...) in both failed conversions to
certmanagerv1.ClusterIssuer and certmanagerv1.Issuer with a call that returns an
irrecoverable error (e.g., common.NewIrrecoverableError(...) or the project’s
equivalent), preserving the descriptive message (e.g., "failed to convert to
ClusterIssuer"/"failed to convert to Issuer") and context ("failed to fetch
issuer") so the reconciler will not retry on these cases.
In `@pkg/controller/trustmanager/utils.go`:
- Around line 113-118: The decodeServiceAccountObjBytes function currently
panics on decode failures; change its signature to return
(*corev1.ServiceAccount, error), decode using runtime.Decode as before, and if
runtime.Decode returns an error return nil and that error; after decoding
perform a type assertion to *corev1.ServiceAccount and if that fails return an
explanatory error (not panic); update any callers of
decodeServiceAccountObjBytes to handle the returned error accordingly so
failures are surfaced and handled instead of crashing the controller.
---
Outside diff comments:
In `@pkg/controller/common/errors.go`:
- Around line 82-91: FromError currently rewraps a MultipleInstanceError into a
RetryRequiredError; change FromError to detect and preserve an existing
MultipleInstanceError by returning it unchanged: inside FromError, before
calling IsIrrecoverableError, check whether err is already a *ReconcileError
(type assertion) and that its Reason equals MultipleInstanceError (or use a
helper IsMultipleInstanceError if available); if so return the original
*ReconcileError; otherwise continue with the existing IsIrrecoverableError ->
NewIrrecoverableError / NewRetryRequiredError flow (functions: FromError,
NewIrrecoverableError, NewRetryRequiredError, MultipleInstanceError,
IsIrrecoverableError).
---
Duplicate comments:
In `@config/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml`:
- Around line 463-468: Update the description string for the field
defaultCAVersion so it references the actual field name (defaultCAVersion)
instead of DefaultCAPackageVersion and clarifies semantics: state that
defaultCAVersion holds the version identifier retrieved from the bundle source
when useDefaultCAs is true and that it will be identical for bundles with the
same certificates; then regenerate the
bundle/manifests/trust.cert-manager.io_bundles.yaml to keep generated manifests
in sync. Ensure you edit the description attached to the defaultCAVersion schema
entry and leave references to useDefaultCAs and the bundle source intact.
In `@pkg/controller/istiocsr/deployments.go`:
- Around line 237-241: The error message in Reconciler.assertIssuerRefExists
uses issuerKind instead of the actual configured value; update the formatted
NewIrrecoverableError call to pass issuerRefKind as the last argument so the
message shows the real configured kind (symbols: assertIssuerRefExists,
issuerRefKind, clusterIssuerKind, issuerKind, errInvalidIssuerRefConfig).
In `@pkg/controller/trustmanager/install_trustmanager.go`:
- Around line 73-80: The code dereferences
TrustManager.Spec.TrustManagerConfig.SecretTargets.Policy and
DefaultCAPackage.Policy without nil checks causing panics; update the blocks in
install_trustmanager.go to guard optional fields by first ensuring
trustManager.Spec.TrustManagerConfig is non-nil and then checking that
SecretTargets and DefaultCAPackage are non-nil before reading their Policy
fields, e.g. only compare/set trustManager.Status.SecretTargetsPolicy when
Spec.TrustManagerConfig.SecretTargets != nil and only compare/set
trustManager.Status.DefaultCAPackagePolicy when
Spec.TrustManagerConfig.DefaultCAPackage != nil (preserve setting changed = true
when you update the status).
In `@pkg/controller/trustmanager/utils.go`:
- Around line 128-133: In updateCondition, the original prependErr is being
dropped and the status update error duplicated when building the aggregate;
change the aggregate to include the original prependErr and the constructed
errUpdate (not err twice). Locate the function Reconciler.updateCondition and
replace the utilerrors.NewAggregate([]error{err, errUpdate}) call with
utilerrors.NewAggregate([]error{prependErr, errUpdate}) (ensuring prependErr is
non-nil in that branch) so the returned aggregate preserves the original error
alongside the status update error.
In `@pkg/operator/setup_manager.go`:
- Around line 188-224: The current addControllerCacheConfig uses objectList[res]
(pointer identity) so different instances of the same resource type won't match;
instead, search objectList for an existing entry by resource type (compare
fmt.Sprintf("%T", key) or reflect.TypeOf(key).String() to resType) before
deciding branch. Replace the direct map lookup if existing, exists :=
objectList[res] with a small loop over objectList keys to find a key whose type
string equals resType (capture that map key as foundKey), then use
objectList[foundKey] as existing and update objectList[foundKey] (or delete +
reinsert using foundKey) when merging selectors; keep the rest of the merge
logic (existingReqs, mergedValues, labels.NewRequirement,
labels.NewSelector().Add) unchanged.
In `@pkg/operator/starter.go`:
- Around line 173-177: The unified controller manager is started with
ctrl.SetupSignalHandler() which ignores the caller's ctx; change the goroutine
to call manager.Start with the provided ctx so the manager stops when ctx is
cancelled (replace the ctrl.SetupSignalHandler() argument to manager.Start with
the caller's ctx used by the other controllers, ensuring the error handling
around manager.Start remains the same).
In `@test/e2e/trustmanager_test.go`:
- Around line 22-25: Rename the misspelled constant
trustManagerServiceAccoutName to trustManagerServiceAccountName and update every
reference to it; specifically modify the constant declaration in
test/e2e/trustmanager_test.go (the const block that defines
trustManagerNamespace, trustManagerServiceAccoutName, trustManagerCommonName)
and replace all usages of trustManagerServiceAccoutName in tests and helpers
(e.g., any test functions, setup helpers, or assertions referencing that symbol)
to the corrected trustManagerServiceAccountName to keep identifiers consistent.
- Around line 44-54: The test's BeforeAll block contains a stale assertion
"Expect(err).NotTo(HaveOccurred())" after the code that set err is commented
out, which is redundant and misleading; remove the trailing
Expect(err).NotTo(HaveOccurred()) or restore the call that sets err (e.g.,
re-enable the patchSubscriptionWithEnvVars call) so that the assertion validates
the most recent operation—look for the BeforeAll function, the local variable
err, and the commented patchSubscriptionWithEnvVars usage to decide whether to
delete the extra Expect or reintroduce the operation that assigns err.
---
Nitpick comments:
In `@bindata/trust-manager/resources/deployment_trust-manager.yml`:
- Around line 40-45: Add a Kubernetes livenessProbe to the same container spec
that currently has readinessProbe (look for readinessProbe, httpGet port: 6060,
path: /readyz) so the container is restarted if it becomes unresponsive; use an
httpGet to a suitable endpoint (e.g., /healthz or /readyz if appropriate) on
port 6060, and configure reasonable initialDelaySeconds, periodSeconds and
failureThreshold values to avoid false positives; ensure the new livenessProbe
block mirrors the readinessProbe placement in the deployment_trust-manager.yml
container spec.
- Line 66: The Deployment has an empty resources block ("resources: {}") which
can cause scheduling and contention problems; update the container spec
(spec.template.spec.containers[].resources) to include at minimum CPU and memory
requests and ideally limits as well (e.g., set requests.cpu, requests.memory and
limits.cpu, limits.memory) to ensure predictable scheduling and resource
capping; choose values appropriate for trust-manager and document rationale in a
comment or chart.
In `@hack/update-trust-manager-manifests.sh`:
- Around line 40-48: After splitting manifests with yq, remove the combined
manifests file to avoid it being moved into bindata; update
hack/update-trust-manager-manifests.sh to delete
${MANIFESTS_PATH}/manifests.yaml (or the appropriate combined file) after the yq
split step and before the mv into bindata occurs so the subsequent mv
${MANIFESTS_PATH}/*.yml bindata/trust-manager/resources only moves per-resource
files; reference the existing MANIFESTS_PATH variable and the yq split step to
place the removal in the correct spot.
In `@pkg/controller/common/client.go`:
- Around line 95-118: The retry lambda in ctrlClientImpl.UpdateWithRetry uses
retry.RetryOnConflict but never checks for context cancellation, so it keeps
retrying after ctx is cancelled; modify the anonymous function passed to
retry.RetryOnConflict (inside UpdateWithRetry) to check ctx.Done()/ctx.Err() at
the start (or via a select) and return ctx.Err() (or context.Canceled) when
cancelled so RetryOnConflict will stop making further attempts; ensure this
check is performed before fetching the latest object and before each retry
iteration.
In `@pkg/controller/common/utils.go`:
- Around line 19-26: Update the HasObjectChanged function comment to explain the
deliberate choice to return false when desired and fetched are different
concrete types rather than treating that as a change; explicitly state that
type-mismatch is considered non-comparable and only metadata differences (via
ObjectMetadataModified) are checked, so callers should handle type-mismatch
(e.g., replacement) elsewhere. Reference the HasObjectChanged function and the
ObjectMetadataModified helper in the comment so future readers understand the
design intent.
- Around line 33-37: ContainsAnnotation currently reads
obj.GetAnnotations()[annotation] without handling a nil map; update
ContainsAnnotation to first retrieve the annotations map (e.g., anns :=
obj.GetAnnotations()), return false if anns == nil, then check the key and
return the boolean. Reference the ContainsAnnotation function and mirror the
nil-handling behavior used in AddAnnotation for consistency.
- Around line 28-31: The function ObjectMetadataModified only compares labels
but its name implies all metadata; either rename it to LabelsModified and update
all call sites (notably HasObjectChanged) to call LabelsModified, or expand
ObjectMetadataModified to compare annotations, finalizers, ownerReferences, and
labels (e.g., compare desired.GetLabels(), desired.GetAnnotations(),
desired.GetFinalizers(), and desired.GetOwnerReferences() via reflect.DeepEqual
against fetched). Choose one approach and apply it consistently: if renaming,
replace ObjectMetadataModified with LabelsModified and update HasObjectChanged;
if expanding, keep the name and add the extra field comparisons in the function.
In `@pkg/controller/istiocsr/utils.go`:
- Around line 232-299: The metadata comparison in hasObjectChanged uses
common.ObjectMetadataModified which currently only compares labels, so
annotation changes on resources (e.g., in hasObjectChanged for
*certmanagerv1.Certificate, *appsv1.Deployment, *corev1.ConfigMap, etc.) will be
ignored and not trigger reconciliation; update the metadata comparison to
include annotations by either extending common.ObjectMetadataModified (add
annotation comparison and rename if needed) or add a new helper like
ObjectMetadataLabelsAndAnnotationsModified and call that from hasObjectChanged
so annotation diffs are detected for all object types handled by
hasObjectChanged.
In `@pkg/controller/trustmanager/controller.go`:
- Around line 225-231: The cleanUp function currently has an untracked TODO
about GA cleanup; create an explicit follow-up issue and reference it in the
source so the work is discoverable: open a GitHub (or tracking) issue describing
the cleanup workflow, then update the TODO comment inside the Reconciler.cleanUp
function to include the issue number/URL and a short action item; also update
the eventRecorder.Eventf call in cleanUp to include the tracking issue ID/URL
and (if present) set a status condition on the TrustManager CR via the
reconciler's status helper (e.g., set a "CleanupPending" or
"CleanupNotImplemented" condition) so operators can see the backlog item from
the object itself.
In `@pkg/operator/platformutil/optional_informer.go`:
- Around line 22-27: The NewOptionalInformer constructor currently accepts a ctx
parameter that is never used; either remove ctx from NewOptionalInformer’s
signature and all call sites (if discovery doesn’t need cancellation), or thread
ctx into the discovery call by passing it into
OptionalInformer.discover/ResourceExists so discover can use ctx for
cancellation; update the function signature and any invocations and modify
discover/ResourceExists implementations to accept and use context.Context if you
choose the second option (referencing NewOptionalInformer and
OptionalInformer.discover/ResourceExists to locate the code).
In `@pkg/operator/setup_manager.go`:
- Around line 53-58: The trustManagerManagedResources slice currently only lists
ServiceAccount; update it to include all resource types managed by TrustManager
per the embedded assets (Deployment, ClusterRole, ClusterRoleBinding, Role,
RoleBinding, Service, Certificate, Issuer, ValidatingWebhookConfiguration) by
adding the corresponding client.Object entries (e.g., &appsv1.Deployment{},
&rbacv1.ClusterRole{}, &rbacv1.ClusterRoleBinding{}, &rbacv1.Role{},
&rbacv1.RoleBinding{}, &corev1.Service{}, &certv1.Certificate{},
&certv1.Issuer{}, &admissionv1.ValidatingWebhookConfiguration{}) to the
trustManagerManagedResources slice and update imports to include the matching
API packages; remove or resolve the TODO once complete.
Comment on lines
373
to
385
| issuerRefKind := strings.ToLower(istiocsr.Spec.IstioCSRConfig.CertManager.IssuerRef.Kind) | ||
| switch issuerRefKind { | ||
| case clusterIssuerKind: | ||
| clusterIssuer, ok := obj.(*certmanagerv1.ClusterIssuer) | ||
| if !ok { | ||
| return FromClientError(fmt.Errorf("failed to convert to ClusterIssuer"), "failed to fetch issuer") | ||
| return common.FromClientError(fmt.Errorf("failed to convert to ClusterIssuer"), "failed to fetch issuer") | ||
| } | ||
| issuerConfig = clusterIssuer.Spec.IssuerConfig | ||
| case issuerKind: | ||
| issuer, ok := obj.(*certmanagerv1.Issuer) | ||
| if !ok { | ||
| return FromClientError(fmt.Errorf("failed to convert to Issuer"), "failed to fetch issuer") | ||
| return common.FromClientError(fmt.Errorf("failed to convert to Issuer"), "failed to fetch issuer") | ||
| } |
There was a problem hiding this comment.
Classify type-assertion failures as irrecoverable.
A failed conversion to *certmanagerv1.ClusterIssuer/*certmanagerv1.Issuer is a programming/data issue, not a client error; returning common.FromClientError makes it retryable and can loop. Prefer an irrecoverable error here.
🧭 Suggested fix
- clusterIssuer, ok := obj.(*certmanagerv1.ClusterIssuer)
- if !ok {
- return common.FromClientError(fmt.Errorf("failed to convert to ClusterIssuer"), "failed to fetch issuer")
- }
+ clusterIssuer, ok := obj.(*certmanagerv1.ClusterIssuer)
+ if !ok {
+ return common.NewIrrecoverableError(errInvalidIssuerRefConfig, "failed to convert to ClusterIssuer")
+ }
@@
- issuer, ok := obj.(*certmanagerv1.Issuer)
- if !ok {
- return common.FromClientError(fmt.Errorf("failed to convert to Issuer"), "failed to fetch issuer")
- }
+ issuer, ok := obj.(*certmanagerv1.Issuer)
+ if !ok {
+ return common.NewIrrecoverableError(errInvalidIssuerRefConfig, "failed to convert to Issuer")
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/istiocsr/deployments.go` around lines 373 - 385, The
type-assertion failures for issuerRefKind handling (issuerRefKind,
clusterIssuerKind, obj) are programming/data issues and should be treated as
irrecoverable; replace the current returns that call common.FromClientError(...)
in both failed conversions to certmanagerv1.ClusterIssuer and
certmanagerv1.Issuer with a call that returns an irrecoverable error (e.g.,
common.NewIrrecoverableError(...) or the project’s equivalent), preserving the
descriptive message (e.g., "failed to convert to ClusterIssuer"/"failed to
convert to Issuer") and context ("failed to fetch issuer") so the reconciler
will not retry on these cases.
Comment on lines
+113
to
+118
| func decodeServiceAccountObjBytes(objBytes []byte) *corev1.ServiceAccount { | ||
| obj, err := runtime.Decode(codecs.UniversalDecoder(corev1.SchemeGroupVersion), objBytes) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return obj.(*corev1.ServiceAccount) |
There was a problem hiding this comment.
Avoid panic on decode failures; return an error instead.
A bad or corrupted asset would crash the controller. This should surface as a handled error.
🛠️ Suggested fix
-func decodeServiceAccountObjBytes(objBytes []byte) *corev1.ServiceAccount {
- obj, err := runtime.Decode(codecs.UniversalDecoder(corev1.SchemeGroupVersion), objBytes)
- if err != nil {
- panic(err)
- }
- return obj.(*corev1.ServiceAccount)
-}
+func decodeServiceAccountObjBytes(objBytes []byte) (*corev1.ServiceAccount, error) {
+ obj, err := runtime.Decode(codecs.UniversalDecoder(corev1.SchemeGroupVersion), objBytes)
+ if err != nil {
+ return nil, fmt.Errorf("failed to decode ServiceAccount: %w", err)
+ }
+ sa, ok := obj.(*corev1.ServiceAccount)
+ if !ok {
+ return nil, fmt.Errorf("decoded object is %T, expected *corev1.ServiceAccount", obj)
+ }
+ return sa, nil
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func decodeServiceAccountObjBytes(objBytes []byte) *corev1.ServiceAccount { | |
| obj, err := runtime.Decode(codecs.UniversalDecoder(corev1.SchemeGroupVersion), objBytes) | |
| if err != nil { | |
| panic(err) | |
| } | |
| return obj.(*corev1.ServiceAccount) | |
| func decodeServiceAccountObjBytes(objBytes []byte) (*corev1.ServiceAccount, error) { | |
| obj, err := runtime.Decode(codecs.UniversalDecoder(corev1.SchemeGroupVersion), objBytes) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to decode ServiceAccount: %w", err) | |
| } | |
| sa, ok := obj.(*corev1.ServiceAccount) | |
| if !ok { | |
| return nil, fmt.Errorf("decoded object is %T, expected *corev1.ServiceAccount", obj) | |
| } | |
| return sa, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/trustmanager/utils.go` around lines 113 - 118, The
decodeServiceAccountObjBytes function currently panics on decode failures;
change its signature to return (*corev1.ServiceAccount, error), decode using
runtime.Decode as before, and if runtime.Decode returns an error return nil and
that error; after decoding perform a type assertion to *corev1.ServiceAccount
and if that fails return an explanatory error (not panic); update any callers of
decodeServiceAccountObjBytes to handle the returned error accordingly so
failures are surfaced and handled instead of crashing the controller.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
pkg/operator/starter.go (1)
163-177:⚠️ Potential issue | 🟠 MajorStart unified manager with the caller’s ctx.
Using
ctrl.SetupSignalHandler()ignores the caller’s cancellation path; ifctxis cancelled (tests or graceful shutdown), the manager keeps running. Passctxdirectly to align lifecycle behavior with the rest ofRunOperator.🔧 Suggested change
go func() { - if err := manager.Start(ctrl.SetupSignalHandler()); err != nil { + if err := manager.Start(ctx); err != nil { ctrl.Log.Error(err, "failed to start unified controller manager") } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/starter.go` around lines 163 - 177, The unified controller manager is being started with ctrl.SetupSignalHandler(), which ignores the caller's ctx and prevents orderly shutdown; update the goroutine that calls manager.Start to pass the incoming ctx (the same context used by RunOperator) instead of ctrl.SetupSignalHandler() so manager.Start(ctx) is used; locate the NewControllerManager/manager.Start call in starter.go and replace the signal handler argument with the function's ctx variable so the manager follows the caller's cancellation path.pkg/operator/setup_manager.go (1)
184-224:⚠️ Potential issue | 🟠 MajorFix shared-resource selector merge to compare types, not pointers.
objectList[res]only matches the exact pointer instance. If both controllers register the same resource type (e.g.,ServiceAccount), the merge path won’t execute, resulting in inconsistent label selectors. Use a type-based lookup to find existing entries and update that key.🛠️ Suggested fix (type-based lookup)
func addControllerCacheConfig(objectList map[client.Object]cache.ByObject, labelValue string, resources []client.Object) error { labelKey := common.ManagedResourceLabelKey for _, res := range resources { - resType := fmt.Sprintf("%T", res) - - if existing, exists := objectList[res]; exists { + resType := fmt.Sprintf("%T", res) + var existingObj client.Object + var existing cache.ByObject + for obj, cfg := range objectList { + if fmt.Sprintf("%T", obj) == resType { + existingObj = obj + existing = cfg + break + } + } + + if existingObj != nil { // Resource already configured by another controller // Merge label values using 'In' operator: app in (value1, value2) existingReqs, _ := existing.Label.Requirements() var existingValues []string @@ - objectList[res] = cache.ByObject{Label: labels.NewSelector().Add(*mergedReq)} + objectList[existingObj] = cache.ByObject{Label: labels.NewSelector().Add(*mergedReq)} setupLog.V(4).Info("merged label selector for shared resource", "type", resType, "values", mergedValues) } else { // First controller to configure this resource labelReq, err := labels.NewRequirement(labelKey, selection.Equals, []string{labelValue}) if err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/setup_manager.go` around lines 184 - 224, The current addControllerCacheConfig uses objectList[res] which compares pointer identity, so registrations of the same resource type by different controller instances won't be detected; change the lookup to be type-based: compute resType (fmt.Sprintf("%T", res) or reflect.TypeOf(res)) and iterate the keys of objectList to find an existing entry whose type string equals resType, then treat that as existing (rename the matched key to matchedRes or existingKey) and merge/update objectList[matchedRes] (not objectList[res]); keep the existing label requirement merge logic (labels.NewRequirement/selection.In) and when no match is found insert objectList[res] as before. Ensure you update references in addControllerCacheConfig (objectList, res, resType, existing) to use the matched key.
🧹 Nitpick comments (3)
pkg/controller/trustmanager/controller.go (2)
174-175: Clarify theReady=Falsecondition reason.Using
v1alpha1.ReasonReadyas the reason when settingReady=Falseis semantically confusing. Consider using a more descriptive reason likeReasonFailedorReasonNotReadyto clearly indicate the failure state, consistent with howDegraded=TrueusesReasonFailedon line 174.♻️ Suggested fix
- readyChanged := trustManager.Status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, v1alpha1.ReasonReady, "") + readyChanged := trustManager.Status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, v1alpha1.ReasonFailed, "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/controller.go` around lines 174 - 175, The Ready=False condition currently uses v1alpha1.ReasonReady which is confusing; update the call to trustManager.Status.SetCondition for v1alpha1.Ready to use a failure-specific reason (e.g., v1alpha1.ReasonFailed or v1alpha1.ReasonNotReady) to match the Degraded=True usage and clearly indicate the error state when reconciliation fails.
56-62: Consider passing context through method calls instead of storing it.Storing
context.Background()in the struct means helper methods (r.ctx) won't respect timeouts or cancellation from the actual reconciliation context passed toReconcile(ctx, req). This pattern exists in similar controllers (e.g., istiocsr), but for better cancellation semantics, consider threading the context through method parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/controller.go` around lines 56 - 62, The Reconciler currently stores context.Background() in the ctx field which prevents helper methods from observing cancellation/timeouts from Reconcile(ctx, req); remove the stored ctx from the Reconciler struct and from the NewReconciler return (do not set ctx = context.Background()), then update all helper methods and call sites (methods referenced by Reconciler, any functions that currently use r.ctx) to accept a context parameter and be invoked with the reconciliation context passed into Reconcile(ctx, req) so cancellation and deadlines propagate correctly.pkg/controller/trustmanager/serviceaccounts.go (1)
29-35: Asset decoding relies on panic-on-error behavior.This function depends on
decodeServiceAccountObjByteswhich panics on decode failure (as flagged in utils.go). The current behavior is consistent withMustAssetfrom bindata, which also panics. This fail-fast approach is acceptable for embedded assets that are validated at build time, but consider propagating errors for better debuggability in production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/serviceaccounts.go` around lines 29 - 35, The function getServiceAccountObject currently uses decodeServiceAccountObjBytes and assets.MustAsset which panic on error; change getServiceAccountObject to return (*corev1.ServiceAccount, error), call the non-panicking asset/decoder (or wrap decodeServiceAccountObjBytes to return (obj, err)), check and return any decode/asset errors instead of letting them panic, and update all callers to handle the error; refer to getServiceAccountObject, decodeServiceAccountObjBytes, serviceAccountAssetName and assets.MustAsset when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/trustmanager/utils.go`:
- Around line 203-212: getResourceLabels currently copies user labels first then
controllerDefaultResourceLabels, causing defaults to overwrite user-specified
keys; reverse the merge so defaults are copied first and then user labels are
copied last. In getResourceLabels, copy controllerDefaultResourceLabels into
resourceLabels first (using maps.Copy(resourceLabels,
controllerDefaultResourceLabels)) and then, if present, copy
trustManager.Spec.ControllerConfig.Labels into resourceLabels so user-provided
keys in trustManager.Spec.ControllerConfig.Labels override the
controllerDefaultResourceLabels.
In `@pkg/operator/platformutil/optional_informer_test.go`:
- Around line 15-16: The dummyInformerInit function currently returns an
anonymous struct literal (struct{}{}) which doesn't match the named
interface/type fakeInformerFactoryStub; change the return value to the named
stub type (return fakeInformerFactoryStub{} or an appropriate value of that
type) so the dummyInformerInit signature and returned value compile and satisfy
fakeInformerFactoryStub.
In `@pkg/operator/platformutil/platform_checker.go`:
- Around line 58-62: In the OpenShift branch, add a defensive nil-check for
p.configGetter before calling p.configGetter.FeatureGates(): if p.configGetter
== nil then return the same triple-typed error response (e.g. return false,
"failed to check FeatureSet", fmt.Errorf("configGetter is nil: cannot get
FeatureGate")) so it doesn't panic; update the block around the FeatureGates()
call (the code that assigns featureGate, err :=
p.configGetter.FeatureGates().Get(...)) to first verify p.configGetter and only
call FeatureGates() when non-nil.
---
Duplicate comments:
In `@pkg/operator/setup_manager.go`:
- Around line 184-224: The current addControllerCacheConfig uses objectList[res]
which compares pointer identity, so registrations of the same resource type by
different controller instances won't be detected; change the lookup to be
type-based: compute resType (fmt.Sprintf("%T", res) or reflect.TypeOf(res)) and
iterate the keys of objectList to find an existing entry whose type string
equals resType, then treat that as existing (rename the matched key to
matchedRes or existingKey) and merge/update objectList[matchedRes] (not
objectList[res]); keep the existing label requirement merge logic
(labels.NewRequirement/selection.In) and when no match is found insert
objectList[res] as before. Ensure you update references in
addControllerCacheConfig (objectList, res, resType, existing) to use the matched
key.
In `@pkg/operator/starter.go`:
- Around line 163-177: The unified controller manager is being started with
ctrl.SetupSignalHandler(), which ignores the caller's ctx and prevents orderly
shutdown; update the goroutine that calls manager.Start to pass the incoming ctx
(the same context used by RunOperator) instead of ctrl.SetupSignalHandler() so
manager.Start(ctx) is used; locate the NewControllerManager/manager.Start call
in starter.go and replace the signal handler argument with the function's ctx
variable so the manager follows the caller's cancellation path.
---
Nitpick comments:
In `@pkg/controller/trustmanager/controller.go`:
- Around line 174-175: The Ready=False condition currently uses
v1alpha1.ReasonReady which is confusing; update the call to
trustManager.Status.SetCondition for v1alpha1.Ready to use a failure-specific
reason (e.g., v1alpha1.ReasonFailed or v1alpha1.ReasonNotReady) to match the
Degraded=True usage and clearly indicate the error state when reconciliation
fails.
- Around line 56-62: The Reconciler currently stores context.Background() in the
ctx field which prevents helper methods from observing cancellation/timeouts
from Reconcile(ctx, req); remove the stored ctx from the Reconciler struct and
from the NewReconciler return (do not set ctx = context.Background()), then
update all helper methods and call sites (methods referenced by Reconciler, any
functions that currently use r.ctx) to accept a context parameter and be invoked
with the reconciliation context passed into Reconcile(ctx, req) so cancellation
and deadlines propagate correctly.
In `@pkg/controller/trustmanager/serviceaccounts.go`:
- Around line 29-35: The function getServiceAccountObject currently uses
decodeServiceAccountObjBytes and assets.MustAsset which panic on error; change
getServiceAccountObject to return (*corev1.ServiceAccount, error), call the
non-panicking asset/decoder (or wrap decodeServiceAccountObjBytes to return
(obj, err)), check and return any decode/asset errors instead of letting them
panic, and update all callers to handle the error; refer to
getServiceAccountObject, decodeServiceAccountObjBytes, serviceAccountAssetName
and assets.MustAsset when applying the change.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (28)
api/operator/v1alpha1/features.gobundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/rbac/role.yamlpkg/controller/common/constants.gopkg/controller/deployment/cert_manager_cainjector_deployment.gopkg/controller/deployment/cert_manager_controller_deployment.gopkg/controller/deployment/cert_manager_controller_set.gopkg/controller/deployment/cert_manager_webhook_deployment.gopkg/controller/deployment/generic_deployment_controller.gopkg/controller/istiocsr/controller.gopkg/controller/trustmanager/constants.gopkg/controller/trustmanager/controller.gopkg/controller/trustmanager/install_trustmanager.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/utils.gopkg/features/features_test.gopkg/operator/optionalinformer/optional_informer.gopkg/operator/optionalinformer/optional_informer_test.gopkg/operator/platformutil/discovery.gopkg/operator/platformutil/discovery_test.gopkg/operator/platformutil/optional_informer.gopkg/operator/platformutil/optional_informer_test.gopkg/operator/platformutil/platform_checker.gopkg/operator/platformutil/platform_checker_test.gopkg/operator/platformutil/testing_helpers_test.gopkg/operator/setup_manager.gopkg/operator/starter.gotest/e2e/trustmanager_test.go
💤 Files with no reviewable changes (2)
- pkg/operator/optionalinformer/optional_informer.go
- pkg/operator/optionalinformer/optional_informer_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/controller/deployment/cert_manager_controller_set.go
- pkg/operator/platformutil/discovery.go
- test/e2e/trustmanager_test.go
- api/operator/v1alpha1/features.go
- pkg/controller/deployment/cert_manager_controller_deployment.go
- pkg/controller/trustmanager/constants.go
- pkg/controller/deployment/generic_deployment_controller.go
- pkg/operator/platformutil/optional_informer.go
- pkg/features/features_test.go
Comment on lines
+203
to
+212
| // getResourceLabels returns the labels to apply to all resources created by the controller. | ||
| // It merges user-specified labels with the controller's default labels. | ||
| func getResourceLabels(trustManager *v1alpha1.TrustManager) map[string]string { | ||
| resourceLabels := make(map[string]string) | ||
| if len(trustManager.Spec.ControllerConfig.Labels) != 0 { | ||
| maps.Copy(resourceLabels, trustManager.Spec.ControllerConfig.Labels) | ||
| } | ||
| maps.Copy(resourceLabels, controllerDefaultResourceLabels) | ||
| return resourceLabels | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what labels are in controllerDefaultResourceLabels to understand potential conflicts
rg -n 'controllerDefaultResourceLabels' --type go -A5Repository: openshift/cert-manager-operator
Length of output: 11106
🏁 Script executed:
#!/bin/bash
# Look for tests related to getResourceLabels to understand intended behavior
rg -n 'getResourceLabels|TestGetResourceLabels' --type go -A10Repository: openshift/cert-manager-operator
Length of output: 2374
🏁 Script executed:
#!/bin/bash
# Check for any documentation or comments about label merging behavior
rg -n 'ControllerConfig.*Labels|user.*labels.*override|default.*labels.*override' --type go -A3 -B3Repository: openshift/cert-manager-operator
Length of output: 5694
Fix label merge order so user-specified labels override controller defaults.
Currently, maps.Copy(resourceLabels, controllerDefaultResourceLabels) on line 210 copies controller defaults after user labels, causing controller defaults to override any user-specified labels with matching keys. The test expectations in pkg/controller/istiocsr/install_instiocsr_test.go:27-30 show that user labels should take precedence. Reverse the copy order so user labels are applied last.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/trustmanager/utils.go` around lines 203 - 212,
getResourceLabels currently copies user labels first then
controllerDefaultResourceLabels, causing defaults to overwrite user-specified
keys; reverse the merge so defaults are copied first and then user labels are
copied last. In getResourceLabels, copy controllerDefaultResourceLabels into
resourceLabels first (using maps.Copy(resourceLabels,
controllerDefaultResourceLabels)) and then, if present, copy
trustManager.Spec.ControllerConfig.Labels into resourceLabels so user-provided
keys in trustManager.Spec.ControllerConfig.Labels override the
controllerDefaultResourceLabels.
Comment on lines
+15
to
+16
| dummyInformerInit := func() fakeInformerFactoryStub { | ||
| return struct{}{} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "optional_informer_test.go" -type fRepository: openshift/cert-manager-operator
Length of output: 128
🏁 Script executed:
cd pkg/operator/platformutil && head -30 optional_informer_test.goRepository: openshift/cert-manager-operator
Length of output: 853
🏁 Script executed:
grep -n "fakeInformerFactoryStub" pkg/operator/platformutil/optional_informer_test.go | head -20Repository: openshift/cert-manager-operator
Length of output: 174
🏁 Script executed:
cd pkg/operator/platformutil && grep -A 50 "dummyInformerInit" optional_informer_test.go | head -60Repository: openshift/cert-manager-operator
Length of output: 1468
🏁 Script executed:
git log --oneline --all -- pkg/operator/platformutil/optional_informer_test.go | head -5Repository: openshift/cert-manager-operator
Length of output: 107
🏁 Script executed:
git show HEAD:pkg/operator/platformutil/optional_informer_test.go | grep -A 5 "dummyInformerInit"Repository: openshift/cert-manager-operator
Length of output: 565
Return the named stub type to avoid a compile error.
struct{}{} is an anonymous type and won't satisfy fakeInformerFactoryStub.
- dummyInformerInit := func() fakeInformerFactoryStub {
- return struct{}{}
+ dummyInformerInit := func() fakeInformerFactoryStub {
+ return fakeInformerFactoryStub{}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dummyInformerInit := func() fakeInformerFactoryStub { | |
| return struct{}{} | |
| dummyInformerInit := func() fakeInformerFactoryStub { | |
| return fakeInformerFactoryStub{} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/platformutil/optional_informer_test.go` around lines 15 - 16,
The dummyInformerInit function currently returns an anonymous struct literal
(struct{}{}) which doesn't match the named interface/type
fakeInformerFactoryStub; change the return value to the named stub type (return
fakeInformerFactoryStub{} or an appropriate value of that type) so the
dummyInformerInit signature and returned value compile and satisfy
fakeInformerFactoryStub.
Comment on lines
+58
to
+62
| // OpenShift - check FeatureSet | ||
| featureGate, err := p.configGetter.FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return false, "failed to check FeatureSet", fmt.Errorf("failed to get featuregate/cluster: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and size
wc -l pkg/operator/platformutil/platform_checker.goRepository: openshift/cert-manager-operator
Length of output: 123
🏁 Script executed:
# Read the full file to understand the context
cat -n pkg/operator/platformutil/platform_checker.goRepository: openshift/cert-manager-operator
Length of output: 3186
🏁 Script executed:
# Search for NewPlatformChecker constructor and its signature
rg -nP --type=go 'func NewPlatformChecker' pkg/operator/platformutil/Repository: openshift/cert-manager-operator
Length of output: 260
🏁 Script executed:
# Find all call sites of NewPlatformChecker across the codebase
rg -nP --type=go 'NewPlatformChecker\s*\(' --max-count=20Repository: openshift/cert-manager-operator
Length of output: 1328
🏁 Script executed:
# Read the test file to understand test scenarios
cat -n pkg/operator/platformutil/platform_checker_test.goRepository: openshift/cert-manager-operator
Length of output: 6953
🏁 Script executed:
# Examine the production call at starter.go:150 to see context
sed -n '140,160p' pkg/operator/starter.goRepository: openshift/cert-manager-operator
Length of output: 1188
🏁 Script executed:
# Check if there's any nil validation for configClient before creating PlatformChecker
rg -nP --type=go -B5 -A5 'NewPlatformChecker.*configClient.ConfigV1' pkg/operator/starter.goRepository: openshift/cert-manager-operator
Length of output: 765
Add nil check for configGetter before use on OpenShift path.
The code at line 59 calls p.configGetter.FeatureGates() without validating that configGetter is not nil. While the production call site (starter.go:150) passes configClient.ConfigV1() which should be valid, the constructor accepts nil and there is no defensive guard. If OpenShift is detected and configGetter is nil, this will panic.
Proposed fix
// OpenShift - check FeatureSet
+ if p.configGetter == nil {
+ return false, "failed to check FeatureSet", fmt.Errorf("feature gate client is nil")
+ }
featureGate, err := p.configGetter.FeatureGates().Get(ctx, "cluster", metav1.GetOptions{})
if err != nil {
return false, "failed to check FeatureSet", fmt.Errorf("failed to get featuregate/cluster: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // OpenShift - check FeatureSet | |
| featureGate, err := p.configGetter.FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return false, "failed to check FeatureSet", fmt.Errorf("failed to get featuregate/cluster: %w", err) | |
| } | |
| // OpenShift - check FeatureSet | |
| if p.configGetter == nil { | |
| return false, "failed to check FeatureSet", fmt.Errorf("feature gate client is nil") | |
| } | |
| featureGate, err := p.configGetter.FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return false, "failed to check FeatureSet", fmt.Errorf("failed to get featuregate/cluster: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/platformutil/platform_checker.go` around lines 58 - 62, In the
OpenShift branch, add a defensive nil-check for p.configGetter before calling
p.configGetter.FeatureGates(): if p.configGetter == nil then return the same
triple-typed error response (e.g. return false, "failed to check FeatureSet",
fmt.Errorf("configGetter is nil: cannot get FeatureGate")) so it doesn't panic;
update the block around the FeatureGates() call (the code that assigns
featureGate, err := p.configGetter.FeatureGates().Get(...)) to first verify
p.configGetter and only call FeatureGates() when non-nil.
Contributor
|
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. |
Signed-off-by: chiragkyal <ckyal@redhat.com>
Contributor
|
@chiragkyal: The following tests 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
2 participants
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.
Summary by CodeRabbit
New Features
Chores
Tests