CM-867: Implement trust-manager resource reconcilers#379
Merged
openshift-merge-bot[bot] merged 7 commits intoopenshift:masterfrom Mar 24, 2026
Merged
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds TrustManager support: new image/version env vars and CSV entries; implements a TrustManager controller (service accounts, RBAC, services, deployment, issuer/certificate, validating webhook), shared validation/util helpers, scheme registration, RBAC role additions, and extensive unit and e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler as TrustManager<br/>Reconciler
participant Controller as Kubernetes<br/>API
participant ServiceAccount as ServiceAccount
participant RBAC as RBAC Resources
participant Deployment as Deployment
participant Services as Services
participant Issuer as Issuer/<br/>Certificate
participant Webhook as Webhook<br/>Config
Reconciler->>Reconciler: reconcileTrustManagerDeployment()
Reconciler->>Reconciler: Validate trust namespace
Reconciler->>ServiceAccount: createOrApplyServiceAccounts()
ServiceAccount->>Controller: Apply/Patch ServiceAccount
Reconciler->>RBAC: createOrApplyRBACResources()
RBAC->>Controller: Apply ClusterRole/Binding
RBAC->>Controller: Apply Namespaced Role/Binding
RBAC->>Controller: Apply LeaderElection Role/Binding
Reconciler->>Services: createOrApplyServices()
Services->>Controller: Apply Webhook Service
Services->>Controller: Apply Metrics Service
Reconciler->>Issuer: createOrApplyIssuer()
Issuer->>Controller: Apply Issuer
Reconciler->>Issuer: createOrApplyCertificate()
Issuer->>Controller: Apply Certificate
Reconciler->>Deployment: createOrApplyDeployment()
Deployment->>Controller: Apply Deployment with config
Reconciler->>Webhook: createOrApplyValidatingWebhookConfiguration()
Webhook->>Controller: Apply WebhookConfiguration
Reconciler->>Controller: updateStatusObservedState()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-merge-robot
added
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
62-62:⚠️ Potential issue | 🟡 MinorRemove extra quotes from TRUST_MANAGER_VERSION.
The version value has unnecessary quotes that are inconsistent with other version definitions (e.g.,
CERT_MANAGER_VERSION,ISTIO_CSR_VERSION). This may cause issues when the variable is expanded in shell commands.🔧 Proposed fix
-TRUST_MANAGER_VERSION ?= "v0.20.3" +TRUST_MANAGER_VERSION ?= v0.20.3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 62, The TRUST_MANAGER_VERSION Makefile variable is defined with extra quotes; update the TRUST_MANAGER_VERSION assignment in the Makefile so it matches other version vars (e.g., CERT_MANAGER_VERSION, ISTIO_CSR_VERSION) by removing the surrounding quotes and using TRUST_MANAGER_VERSION ?= v0.20.3 so shell expansions and comparisons behave consistently.
🧹 Nitpick comments (7)
pkg/controller/common/client.go (1)
100-104: Reflection-based type creation may not work for all object types.The reflection logic creates a new instance via
reflect.New(reflect.TypeOf(obj).Elem()). This assumesobjis always a pointer to a struct. Ifobjis a non-pointer type or an interface without an underlying struct, this could panic or produce unexpected behavior.Consider adding a nil check or documenting the constraint that
objmust be a non-nil pointer to a struct.💡 Suggested defensive check
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 { + if obj == nil || reflect.TypeOf(obj).Kind() != reflect.Ptr { + return fmt.Errorf("obj must be a non-nil pointer") + } currentInterface := reflect.New(reflect.TypeOf(obj).Elem()).Interface() current, ok := currentInterface.(client.Object) if !ok { return fmt.Errorf("failed to convert to 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 100 - 104, The reflection code using reflect.New(reflect.TypeOf(obj).Elem()) can panic or behave unexpectedly if obj is nil, not a pointer, or not pointing to a struct; update the logic around currentInterface/current to validate obj first (ensure obj != nil, TypeOf(obj).Kind() == reflect.Ptr and TypeOf(obj).Elem().Kind() == reflect.Struct) and return a clear error if the constraint is violated, or document that obj must be a non-nil pointer to a struct; keep the existing conversion to client.Object (current, ok) after these checks.pkg/controller/common/errors.go (2)
45-55: Simplify redundant string conversion.
fmt.Sprint(err.Error())is equivalent toerr.Error(). Thefmt.Sprintcall is unnecessary.♻️ Proposed fix
func NewMultipleInstanceError(err error) *ReconcileError { if err == nil { return nil } return &ReconcileError{ Reason: MultipleInstanceError, - Message: fmt.Sprint(err.Error()), + Message: err.Error(), Err: err, } }🤖 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 45 - 55, The NewMultipleInstanceError function redundantly wraps err.Error() with fmt.Sprint; replace fmt.Sprint(err.Error()) with err.Error() in the returned ReconcileError (in function NewMultipleInstanceError) so Message is set directly from err.Error(); keep the existing nil-check and preserve Reason: MultipleInstanceError and Err: err.
24-31: Consider addingUnwrap()method for proper error chain support.The
ReconcileErrorstruct wraps an underlying error but doesn't implementUnwrap(). This meanserrors.Is()anderrors.As()won't traverse to the wrapped error, which may limit error inspection capabilities.♻️ Proposed addition
Add after the
Error()method:// Unwrap returns the underlying error for error chain traversal. func (e *ReconcileError) Unwrap() error { return e.Err }🤖 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 24 - 31, ReconcileError wraps an underlying error but lacks an Unwrap method, preventing errors.Is()/errors.As() from traversing the wrapped error; add a method func (e *ReconcileError) Unwrap() error that returns e.Err (place it after the existing Error() method) so standard error chain functions can inspect the underlying error; ensure the receiver is a pointer to ReconcileError and it simply returns the Err field.pkg/controller/istiocsr/utils.go (1)
136-230: Consider migrating decode functions to usecommon.DecodeObjBytes.The local
decode*ObjBytesfunctions (e.g.,decodeDeploymentObjBytes,decodeClusterRoleObjBytes, etc.) follow the same pattern and could potentially be replaced with calls tocommon.DecodeObjBytes[T]. This would further reduce duplication.However, this is a non-blocking suggestion since the current implementation works correctly.
♻️ Example migration for decodeDeploymentObjBytes
func decodeDeploymentObjBytes(objBytes []byte) *appsv1.Deployment { - obj, err := runtime.Decode(codecs.UniversalDecoder(appsv1.SchemeGroupVersion), objBytes) - if err != nil { - panic(err) - } - deployment, ok := obj.(*appsv1.Deployment) - if !ok { - panic("failed to convert to *appsv1.Deployment") - } - return deployment + return common.DecodeObjBytes[*appsv1.Deployment](codecs, appsv1.SchemeGroupVersion, objBytes) }🤖 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 136 - 230, The decodeXObjBytes functions (decodeDeploymentObjBytes, decodeClusterRoleObjBytes, decodeClusterRoleBindingObjBytes, decodeRoleObjBytes, decodeRoleBindingObjBytes, decodeServiceObjBytes, decodeServiceAccountObjBytes, decodeCertificateObjBytes) are duplicative; replace their bodies with calls to the generic helper common.DecodeObjBytes[T] (e.g., common.DecodeObjBytes[appsv1.Deployment](objBytes)) or remove the redundant functions and call common.DecodeObjBytes[T] where they’re used, ensure you import the common package, and update callers to the generic version so duplication is eliminated while preserving panic/error behavior.pkg/controller/common/utils.go (2)
37-40: Function name is misleading - only compares labels, not full metadata.
ObjectMetadataModifiedsuggests it compares all object metadata, but it only compares labels. Consider renaming toObjectLabelsModifiedfor clarity, or expanding to compare other metadata fields (annotations, etc.) if that's the intended behavior.♻️ Option 1: Rename for clarity
-// ObjectMetadataModified compares the labels of two objects and returns true if they differ. -func ObjectMetadataModified(desired, fetched client.Object) bool { +// ObjectLabelsModified compares the labels of two objects and returns true if they differ. +func ObjectLabelsModified(desired, fetched client.Object) bool { return !reflect.DeepEqual(desired.GetLabels(), fetched.GetLabels()) }🤖 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 37 - 40, The function ObjectMetadataModified only compares labels but the name implies full metadata comparison; rename ObjectMetadataModified to ObjectLabelsModified and update all callers to use ObjectLabelsModified (keeping the implementation that returns !reflect.DeepEqual(desired.GetLabels(), fetched.GetLabels())); alternatively, if you intended to compare more metadata, expand the function (e.g., add comparisons for desired.GetAnnotations()/fetched.GetAnnotations() and any other metadata fields) and keep or rename to ObjectMetadataModified accordingly—ensure to update references to the function name (ObjectMetadataModified/ObjectLabelsModified) and adjust tests or callers to match the chosen change.
63-75: Panic-on-failure pattern is acceptable for static asset decoding but should be documented.The function panics on decode failure or type mismatch. While this is appropriate for static assets loaded at initialization, callers should be aware of this behavior.
The panic message on line 68 uses
*new(T)which works but could be clearer.♻️ Suggested doc enhancement
// DecodeObjBytes decodes raw YAML/JSON bytes into a typed Kubernetes object. -// Panics on decode failure or type mismatch. +// Panics on decode failure or type mismatch. Use only for static assets +// loaded at initialization time where failure indicates a programmer error. func DecodeObjBytes[T runtime.Object](codecs serializer.CodecFactory, gv schema.GroupVersion, objBytes []byte) T {🤖 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 63 - 75, The DecodeObjBytes function panics on decode/type mismatch which is OK for static asset decoding but needs explicit documentation and clearer panic messages; update the function comment for DecodeObjBytes to state that it will panic on decode failure or type mismatch and is intended for static/initialization use, and replace the current panic messages that use *new(T) with a clearer type representation (use a reflect-based or fmt-based type string for T) so the error text unambiguously shows the expected type in both decode and conversion failure cases.pkg/controller/trustmanager/install_trustmanager.go (1)
59-61: Consider usingcommon.FromClientErrorfor consistency.The error from
updateStatusObservedStateis wrapped withfmt.Errorf, while other reconciliation errors in this file usecommon.FromClientErrororcommon.NewIrrecoverableError. Consider using the common error utilities for consistent error classification and retry behavior.♻️ Suggested change
if err := r.updateStatusObservedState(trustManager); err != nil { - return fmt.Errorf("failed to update status observed state: %w", err) + return common.FromClientError(err, "failed to update status observed state") }🤖 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 59 - 61, The current return wraps the updateStatusObservedState error with fmt.Errorf; replace that wrapping with the project's error helper so the error is classified consistently and preserves retry semantics—call common.FromClientError(err, "failed to update status observed state") (or common.NewIrrecoverableError if the error should not be retried) where updateStatusObservedState is invoked so the reconciler uses the same error utilities as other handlers.
🤖 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`:
- Line 28: The PreRelease values are inconsistent: FeatureTrustManager is set
with PreRelease: "TechPreview" while another feature uses featuregate.GA; make
them consistent by either (a) introducing a package-level constant (e.g., const
PreReleaseTechPreview = "TechPreview") and use PreRelease: PreReleaseTechPreview
for FeatureTrustManager, or (b) change the other feature that currently uses
featuregate.GA to use the equivalent string literal so all PreRelease fields use
the same string style; update references to the PreRelease value in features.go
(look for FeatureTrustManager and the feature using featuregate.GA) accordingly.
In `@pkg/controller/common/utils.go`:
- Around line 28-35: The inconsistency is that HasObjectChanged(desired,
fetched) in pkg/controller/common/utils.go returns false when types differ,
while the local hasObjectChanged in istiocsr/utils.go panics on type mismatch;
fix by aligning behavior: update hasObjectChanged in istiocsr/utils.go to
delegate to the common HasObjectChanged (or perform the same reflect.TypeOf
check and return false) instead of panicking, so both functions have identical
type-mismatch handling and avoid duplicated logic.
In `@pkg/controller/istiocsr/controller.go`:
- Around line 84-85: The predicate uses common.ManagedResourceLabelKey (which
resolves to the generic "app" key) to detect operator-owned resources; replace
that with a new operator-specific label constant (e.g., IstioCSRManagedLabelKey
or ManagedByIstioCSRKey) defined in the constants file and use it everywhere you
currently check objLabels[common.ManagedResourceLabelKey] ==
RequestEnqueueLabelValue (including the labelOk closure and the other predicate
block around RequestEnqueueLabelValue). Add the new constant to the constants
file, update the checks in controller.go to reference the new constant (and
RequestEnqueueLabelValue), and run/update any tests that assert label-based
behavior to use the new operator-specific label.
In `@pkg/controller/istiocsr/deployments.go`:
- Around line 376-385: The type-assertion failures for clusterIssuer and issuer
are internal invariant violations and should return an irrecoverable error
instead of a retryable client error; update the two branches that check
clusterIssuer (variable clusterIssuer) and issuer (variable issuer, case
issuerKind) to return an irrecoverable error (e.g., use the project's
irrecoverable error constructor such as common.NewIrrecoverableError or
equivalent) rather than common.FromClientError so these failures won't be
treated as retryable.
- Around line 321-324: r.Get can fail transiently so don't wrap every error as
irrecoverable; instead, after calling r.Get(ctx, sourceConfigMapKey,
sourceConfigMap) check the returned err and only return
common.NewIrrecoverableError for the actual misconfiguration case (e.g., when
the ConfigMap exists but is malformed/missing expected keys) and for all other
client/API errors return common.FromClientError(err, "failed to fetch CA
certificate ConfigMap %s/%s", sourceConfigMapKey.Namespace,
sourceConfigMapKey.Name); in short: replace the unconditional
NewIrrecoverableError usage with a branch that detects the misconfiguration
condition and uses NewIrrecoverableError there, and uses common.FromClientError
for transient/client errors from r.Get.
In `@pkg/controller/trustmanager/deployments.go`:
- Around line 233-249: The helper containerPortsMatch currently only checks
ContainerPort and must be updated to compare the full corev1.ContainerPort spec
to detect drift; modify containerPortsMatch to require same length and for each
desired port find an existing port with equal Name, ContainerPort, HostPort,
Protocol (and HostIP if used) — or simply perform a full struct equality (e.g.
reflect.DeepEqual) between desired and matching existing ports — so
name/protocol/hostPort differences no longer get ignored.
- Around line 55-60: The code currently silently no-ops when the baked-in
container named by trustManagerContainerName is missing, so change the update
helpers (updateDeploymentArgs, updateTLSSecretVolume, updateImage) and/or add a
pre-check in getDeploymentObject to explicitly detect whether a container with
trustManagerContainerName exists and return an error if not; locate the loop or
lookup that references containers in the
Deployment.Spec.Template.Spec.Containers, and if you cannot find a container
matching trustManagerContainerName, return a descriptive error (propagate it up
so getDeploymentObject fails) instead of silently continuing, ensuring a missing
asset container surfaces as a hard error and prevents applying the Deployment
with defaults.
In `@pkg/controller/trustmanager/utils.go`:
- Around line 184-199: The function currently always returns
common.NewMultipleInstanceError(statusMessage) when ignoreProcessing is false,
which incorrectly rejects the winning singleton; change the control flow so that
when this TrustManager instance is the chosen/oldest (i.e., the winning
singleton) you return nil instead of NewMultipleInstanceError. Concretely, after
the ignoreProcessing block check the condition that determines this instance is
the active singleton (use the same logic/flags already computed around
ignoreProcessing/trustManager) and return nil for that case; only return
common.NewMultipleInstanceError(fmt.Errorf("%s", statusMessage)) for non-winning
instances. Ensure the successful update path (updateCondition/updateWithRetry)
remains unchanged and keep the references to trustManager, statusMessage,
updateCondition, addProcessingRejectedAnnotation, and UpdateWithRetry.
- Around line 129-133: The validateTrustManagerConfig function currently rejects
a zero-valued TrustManagerConfig which breaks reconciliation and e2e tests;
update the code so empty/zero TrustManagerConfig is considered valid—either
remove the call to validateTrustManagerConfig from
reconcileTrustManagerDeployment or change validateTrustManagerConfig to always
return nil (accept empty config) and keep any other semantic validations if
needed; ensure the change targets the validateTrustManagerConfig function and
the reconcileTrustManagerDeployment call site so an empty
TrustManager.Spec.TrustManagerConfig passes validation.
In `@pkg/operator/setup_manager.go`:
- Around line 196-230: The map key uses client.Object pointer instances in
addControllerCacheConfig so identical resource types from different controllers
don't match (different pointer addresses), causing label selectors to be
overwritten instead of merged; change the keying to be type-based (e.g., use
reflect.TypeOf(res) or a GVK string) so the lookup/merge logic operates on the
resource type rather than the object pointer, update objectList's key type and
all access sites in addControllerCacheConfig to use that type-based key, and
preserve the existing merge logic (existing.Label.Requirements(),
labels.NewRequirement(...), etc.) so multiple controllers' label values are
combined.
In `@pkg/operator/starter.go`:
- Around line 156-160: The manager is being started with its own signal handler
via ctrl.SetupSignalHandler(), which creates a separate context and can cause
shutdown inconsistencies; change the goroutine that calls manager.Start so it
uses the passed ctx (from controllercmd.ControllerContext) instead of
ctrl.SetupSignalHandler() — i.e., call manager.Start(ctx) (or the appropriate
context variable name) so the unified controller manager respects the parent
context cancellation, and remove/avoid creating an independent signal handler.
Ensure the goroutine and any related shutdown logic reference the same ctx
variable.
In `@test/e2e/trustmanager_test.go`:
- Around line 206-210: The test should fail fast when the controller reports
Degraded=True instead of continuing to poll; replace the current early-return
(return false, nil) inside the degradedCondition check with a failing return
(return false, error) that includes the condition's Reason and Message so CI
shows the cause; use the existing symbols meta.FindStatusCondition,
trustManagerStatus.Conditions, v1alpha1.Degraded and metav1.ConditionTrue and
construct an error (e.g. via fmt.Errorf) containing degradedCondition.Reason and
degradedCondition.Message and add the fmt import if missing.
- Around line 44-58: BeforeAll currently patches the Subscription but never
restores the original UNSUPPORTED_ADDON_FEATURES value; capture the current
value before calling patchSubscriptionWithEnvVars (e.g., read the Subscription
and store its UNSUPPORTED_ADDON_FEATURES into a package-level var) and add a
matching AfterAll that calls patchSubscriptionWithEnvVars to restore that saved
value (or remove the key if it was empty) and then call
waitForDeploymentEnvVarAndRollout to wait for the operator to return to the
original env state; reference BeforeAll, patchSubscriptionWithEnvVars and
waitForDeploymentEnvVarAndRollout when locating where to add the capture and
restore logic.
---
Outside diff comments:
In `@Makefile`:
- Line 62: The TRUST_MANAGER_VERSION Makefile variable is defined with extra
quotes; update the TRUST_MANAGER_VERSION assignment in the Makefile so it
matches other version vars (e.g., CERT_MANAGER_VERSION, ISTIO_CSR_VERSION) by
removing the surrounding quotes and using TRUST_MANAGER_VERSION ?= v0.20.3 so
shell expansions and comparisons behave consistently.
---
Nitpick comments:
In `@pkg/controller/common/client.go`:
- Around line 100-104: The reflection code using
reflect.New(reflect.TypeOf(obj).Elem()) can panic or behave unexpectedly if obj
is nil, not a pointer, or not pointing to a struct; update the logic around
currentInterface/current to validate obj first (ensure obj != nil,
TypeOf(obj).Kind() == reflect.Ptr and TypeOf(obj).Elem().Kind() ==
reflect.Struct) and return a clear error if the constraint is violated, or
document that obj must be a non-nil pointer to a struct; keep the existing
conversion to client.Object (current, ok) after these checks.
In `@pkg/controller/common/errors.go`:
- Around line 45-55: The NewMultipleInstanceError function redundantly wraps
err.Error() with fmt.Sprint; replace fmt.Sprint(err.Error()) with err.Error() in
the returned ReconcileError (in function NewMultipleInstanceError) so Message is
set directly from err.Error(); keep the existing nil-check and preserve Reason:
MultipleInstanceError and Err: err.
- Around line 24-31: ReconcileError wraps an underlying error but lacks an
Unwrap method, preventing errors.Is()/errors.As() from traversing the wrapped
error; add a method func (e *ReconcileError) Unwrap() error that returns e.Err
(place it after the existing Error() method) so standard error chain functions
can inspect the underlying error; ensure the receiver is a pointer to
ReconcileError and it simply returns the Err field.
In `@pkg/controller/common/utils.go`:
- Around line 37-40: The function ObjectMetadataModified only compares labels
but the name implies full metadata comparison; rename ObjectMetadataModified to
ObjectLabelsModified and update all callers to use ObjectLabelsModified (keeping
the implementation that returns !reflect.DeepEqual(desired.GetLabels(),
fetched.GetLabels())); alternatively, if you intended to compare more metadata,
expand the function (e.g., add comparisons for
desired.GetAnnotations()/fetched.GetAnnotations() and any other metadata fields)
and keep or rename to ObjectMetadataModified accordingly—ensure to update
references to the function name (ObjectMetadataModified/ObjectLabelsModified)
and adjust tests or callers to match the chosen change.
- Around line 63-75: The DecodeObjBytes function panics on decode/type mismatch
which is OK for static asset decoding but needs explicit documentation and
clearer panic messages; update the function comment for DecodeObjBytes to state
that it will panic on decode failure or type mismatch and is intended for
static/initialization use, and replace the current panic messages that use
*new(T) with a clearer type representation (use a reflect-based or fmt-based
type string for T) so the error text unambiguously shows the expected type in
both decode and conversion failure cases.
In `@pkg/controller/istiocsr/utils.go`:
- Around line 136-230: The decodeXObjBytes functions (decodeDeploymentObjBytes,
decodeClusterRoleObjBytes, decodeClusterRoleBindingObjBytes, decodeRoleObjBytes,
decodeRoleBindingObjBytes, decodeServiceObjBytes, decodeServiceAccountObjBytes,
decodeCertificateObjBytes) are duplicative; replace their bodies with calls to
the generic helper common.DecodeObjBytes[T] (e.g.,
common.DecodeObjBytes[appsv1.Deployment](objBytes)) or remove the redundant
functions and call common.DecodeObjBytes[T] where they’re used, ensure you
import the common package, and update callers to the generic version so
duplication is eliminated while preserving panic/error behavior.
In `@pkg/controller/trustmanager/install_trustmanager.go`:
- Around line 59-61: The current return wraps the updateStatusObservedState
error with fmt.Errorf; replace that wrapping with the project's error helper so
the error is classified consistently and preserves retry semantics—call
common.FromClientError(err, "failed to update status observed state") (or
common.NewIrrecoverableError if the error should not be retried) where
updateStatusObservedState is invoked so the reconciler uses the same error
utilities as other handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6efa00d3-a92f-40fc-bc0f-c86732c30476
⛔ Files ignored due to path filters (9)
go.work.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/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (50)
Makefileapi/operator/v1alpha1/features.gobundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/manager/manager.yamlconfig/rbac/role.yamlpkg/controller/common/client.gopkg/controller/common/constants.gopkg/controller/common/errors.gopkg/controller/common/fakes/fake_ctrl_client.gopkg/controller/common/utils.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/certificates.gopkg/controller/trustmanager/certificates_test.gopkg/controller/trustmanager/constants.gopkg/controller/trustmanager/controller.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/deployments.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/install_trustmanager.gopkg/controller/trustmanager/rbacs.gopkg/controller/trustmanager/rbacs_test.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/serviceaccounts_test.gopkg/controller/trustmanager/services.gopkg/controller/trustmanager/services_test.gopkg/controller/trustmanager/test_utils.gopkg/controller/trustmanager/utils.gopkg/controller/trustmanager/utils_test.gopkg/controller/trustmanager/webhooks.gopkg/controller/trustmanager/webhooks_test.gopkg/features/features_test.gopkg/operator/setup_manager.gopkg/operator/starter.gotest/e2e/trustmanager_test.gotools/go.mod
chiragkyal
force-pushed
the
trust-manager-reconcilers
branch
from
b48171b to
198894c
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
62-62:⚠️ Potential issue | 🟡 MinorRemove quotes around version string for consistency.
TRUST_MANAGER_VERSIONhas quotes around the value ("v0.20.3") while other version variables likeCERT_MANAGER_VERSIONandISTIO_CSR_VERSIONdon't. This inconsistency may cause the literal quotes to be included in image tags when the variable is expanded.🐛 Proposed fix
-TRUST_MANAGER_VERSION ?= "v0.20.3" +TRUST_MANAGER_VERSION ?= v0.20.3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 62, The TRUST_MANAGER_VERSION variable in the Makefile is set with surrounding quotes which can inject literal quotes into expansions; change TRUST_MANAGER_VERSION ?= "v0.20.3" to an unquoted value TRUST_MANAGER_VERSION ?= v0.20.3 to match other variables (e.g., CERT_MANAGER_VERSION, ISTIO_CSR_VERSION) and ensure image tags or string concatenations don't include stray quotes, then run a quick make target that uses TRUST_MANAGER_VERSION to verify correct expansion.
🧹 Nitpick comments (4)
pkg/controller/trustmanager/test_utils.go (1)
55-62: Consider documenting that tests must inject a client separately.The
testReconcilerfactory creates aReconcilerwithout initializing theCtrlClientfield. Tests calling methods liker.Exists()orr.Patch()will panic unless they separately inject a client. This appears intentional for flexibility, but a brief comment would help future maintainers understand the expected usage pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/test_utils.go` around lines 55 - 62, Add a short comment to the testReconciler factory explaining that it intentionally does not initialize the Reconciler.CtrlClient and that tests must inject a fake or real client before calling client-dependent methods (e.g., Reconciler.Exists, Reconciler.Patch); place the comment immediately above the testReconciler function and reference the Reconciler type and CtrlClient field so future maintainers understand the expected usage pattern.pkg/controller/trustmanager/services_test.go (1)
105-116: Reduce call-order coupling inExistsstubs.Multiple cases rely on
existsCallsequencing to choose webhook vs metrics objects. Prefer branching bykey.Nameso tests stay stable if reconciliation call order changes.Refactor example
- existsCall := 0 m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { - existsCall++ - var svc *corev1.Service - if existsCall == 1 { - svc = getWebhookServiceObject(testResourceLabels(), testResourceAnnotations()) - } else { - svc = getMetricsServiceObject(testResourceLabels(), testResourceAnnotations()) - } + var svc *corev1.Service + switch key.Name { + case trustManagerServiceName: + svc = getWebhookServiceObject(testResourceLabels(), testResourceAnnotations()) + case trustManagerMetricsServiceName: + svc = getMetricsServiceObject(testResourceLabels(), testResourceAnnotations()) + default: + return false, nil + } svc.DeepCopyInto(obj.(*corev1.Service)) return true, nil })Also applies to: 124-136, 149-160, 169-180, 189-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/services_test.go` around lines 105 - 116, The current Exists stub in m.ExistsCalls uses an incremental existsCall counter to choose between webhook vs metrics Service fixtures (existsCall variable and the anonymous function passed to m.ExistsCalls), which couples tests to call order; change the stub to inspect the incoming client.ObjectKey (key.Name) and branch on that (e.g., if key.Name == "<webhook-service-name>" use getWebhookServiceObject(...), else if key.Name == "<metrics-service-name>" use getMetricsServiceObject(...)) so it returns the correct Service into obj.(*corev1.Service) regardless of call sequencing; apply the same key.Name-based branching fix to the other Exists stubs referenced (the blocks around lines 124-136, 149-160, 169-180, 189-200) so all tests are stable when reconciliation call order changes.pkg/controller/trustmanager/deployments_test.go (1)
475-477: Make the “missing image” case deterministic by explicitly clearing the env var.The
setImage=falsebranch currently relies on ambient process env state. Explicitly unset/cleartrustManagerImageNameEnvVarNamein that branch to avoid environment-dependent failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/deployments_test.go` around lines 475 - 477, The test relies on tt.setImage to decide whether to set the env var trustManagerImageNameEnvVarName, but when setImage is false it leaves the process env untouched which can cause nondeterministic failures; in the else branch of that if (where tt.setImage is false) explicitly clear the environment by calling os.Unsetenv(trustManagerImageNameEnvVarName) (or otherwise ensure the variable is removed) so the test no longer depends on ambient process env; update deployments_test.go around the tt.setImage block that currently sets testImage to use this explicit unset when not setting the image.pkg/controller/trustmanager/rbacs_test.go (1)
587-607: Consider failing fast if multiple ServiceAccount subjects exist.The helper iterates through all subjects but only validates the first ServiceAccount found. If multiple ServiceAccount subjects exist unexpectedly, the test would pass on the first match without flagging potential issues.
🔧 Suggested improvement
func assertSubjects(t *testing.T, subjects []rbacv1.Subject, expectedName, expectedNamespace string) { t.Helper() if len(subjects) == 0 { t.Fatal("expected at least one subject") } - found := false + var found *rbacv1.Subject for _, s := range subjects { if s.Kind == "ServiceAccount" { - found = true - if s.Name != expectedName { - t.Errorf("expected subject name %q, got %q", expectedName, s.Name) - } - if s.Namespace != expectedNamespace { - t.Errorf("expected subject namespace %q, got %q", expectedNamespace, s.Namespace) + if found != nil { + t.Error("expected exactly one ServiceAccount subject, found multiple") } + found = &s } } - if !found { + if found == nil { t.Error("expected to find a ServiceAccount subject") + return + } + if found.Name != expectedName { + t.Errorf("expected subject name %q, got %q", expectedName, found.Name) + } + if found.Namespace != expectedNamespace { + t.Errorf("expected subject namespace %q, got %q", expectedNamespace, found.Namespace) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/rbacs_test.go` around lines 587 - 607, The helper assertSubjects currently scans subjects and validates only the first ServiceAccount it finds, which lets tests pass if multiple ServiceAccount subjects exist; update assertSubjects to detect and fail when more than one ServiceAccount is present (or alternatively require exactly one) and validate all ServiceAccount fields deterministically: count ServiceAccount occurrences while iterating the subjects slice, if count == 0 call t.Error as before, if count > 1 call t.Fatalf or t.Errorf to fail fast, and continue to assert that the (single) ServiceAccount's Name and Namespace match expectedName/expectedNamespace; reference the assertSubjects function and its subjects parameter to locate where to add the counting and failure logic.
🤖 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/controller.go`:
- Around line 105-119: The current use of withIgnoreStatusUpdatePredicates
(which is builder.WithPredicates(predicate.GenerationChangedPredicate{},
controllerManagedResources)) on the Deployment, Certificate, and Issuer watches
suppresses metadata-only updates; replace the third argument in Watches for
&appsv1.Deployment{}, &certmanagerv1.Certificate{}, and &certmanagerv1.Issuer{}
to use controllerManagedResourcePredicates (or a custom predicate that also
reacts to label/annotation changes) instead of withIgnoreStatusUpdatePredicates
so metadata-only drift will enqueue reconciles; modify the Watches lines
referencing withIgnoreStatusUpdatePredicates accordingly (or implement and
reference a new predicate function that detects metadata changes if preferred).
---
Outside diff comments:
In `@Makefile`:
- Line 62: The TRUST_MANAGER_VERSION variable in the Makefile is set with
surrounding quotes which can inject literal quotes into expansions; change
TRUST_MANAGER_VERSION ?= "v0.20.3" to an unquoted value TRUST_MANAGER_VERSION ?=
v0.20.3 to match other variables (e.g., CERT_MANAGER_VERSION, ISTIO_CSR_VERSION)
and ensure image tags or string concatenations don't include stray quotes, then
run a quick make target that uses TRUST_MANAGER_VERSION to verify correct
expansion.
---
Nitpick comments:
In `@pkg/controller/trustmanager/deployments_test.go`:
- Around line 475-477: The test relies on tt.setImage to decide whether to set
the env var trustManagerImageNameEnvVarName, but when setImage is false it
leaves the process env untouched which can cause nondeterministic failures; in
the else branch of that if (where tt.setImage is false) explicitly clear the
environment by calling os.Unsetenv(trustManagerImageNameEnvVarName) (or
otherwise ensure the variable is removed) so the test no longer depends on
ambient process env; update deployments_test.go around the tt.setImage block
that currently sets testImage to use this explicit unset when not setting the
image.
In `@pkg/controller/trustmanager/rbacs_test.go`:
- Around line 587-607: The helper assertSubjects currently scans subjects and
validates only the first ServiceAccount it finds, which lets tests pass if
multiple ServiceAccount subjects exist; update assertSubjects to detect and fail
when more than one ServiceAccount is present (or alternatively require exactly
one) and validate all ServiceAccount fields deterministically: count
ServiceAccount occurrences while iterating the subjects slice, if count == 0
call t.Error as before, if count > 1 call t.Fatalf or t.Errorf to fail fast, and
continue to assert that the (single) ServiceAccount's Name and Namespace match
expectedName/expectedNamespace; reference the assertSubjects function and its
subjects parameter to locate where to add the counting and failure logic.
In `@pkg/controller/trustmanager/services_test.go`:
- Around line 105-116: The current Exists stub in m.ExistsCalls uses an
incremental existsCall counter to choose between webhook vs metrics Service
fixtures (existsCall variable and the anonymous function passed to
m.ExistsCalls), which couples tests to call order; change the stub to inspect
the incoming client.ObjectKey (key.Name) and branch on that (e.g., if key.Name
== "<webhook-service-name>" use getWebhookServiceObject(...), else if key.Name
== "<metrics-service-name>" use getMetricsServiceObject(...)) so it returns the
correct Service into obj.(*corev1.Service) regardless of call sequencing; apply
the same key.Name-based branching fix to the other Exists stubs referenced (the
blocks around lines 124-136, 149-160, 169-180, 189-200) so all tests are stable
when reconciliation call order changes.
In `@pkg/controller/trustmanager/test_utils.go`:
- Around line 55-62: Add a short comment to the testReconciler factory
explaining that it intentionally does not initialize the Reconciler.CtrlClient
and that tests must inject a fake or real client before calling client-dependent
methods (e.g., Reconciler.Exists, Reconciler.Patch); place the comment
immediately above the testReconciler function and reference the Reconciler type
and CtrlClient field so future maintainers understand the expected usage
pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1eb27f9-4095-4c34-8155-591dc092dfc6
📒 Files selected for processing (24)
Makefilebundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/manager/manager.yamlpkg/controller/common/utils.gopkg/controller/trustmanager/certificates.gopkg/controller/trustmanager/certificates_test.gopkg/controller/trustmanager/constants.gopkg/controller/trustmanager/controller.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/deployments.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/install_trustmanager.gopkg/controller/trustmanager/rbacs.gopkg/controller/trustmanager/rbacs_test.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/serviceaccounts_test.gopkg/controller/trustmanager/services.gopkg/controller/trustmanager/services_test.gopkg/controller/trustmanager/test_utils.gopkg/controller/trustmanager/utils.gopkg/controller/trustmanager/utils_test.gopkg/controller/trustmanager/webhooks.gopkg/controller/trustmanager/webhooks_test.gopkg/operator/setup_manager.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/controller/trustmanager/webhooks_test.go
- pkg/controller/trustmanager/serviceaccounts_test.go
- pkg/operator/setup_manager.go
- pkg/controller/trustmanager/webhooks.go
chiragkyal
force-pushed
the
trust-manager-reconcilers
branch
from
198894c to
53cb39e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
318-319: Make local-run debug/feature flags configurable instead of commented.Line 318 and Line 319 are useful but currently require file edits. Consider routing them through a variable so developers can toggle at invocation time.
♻️ Suggested tweak
-# --unsupported-addon-features="TrustManager=true" \ -# --v=4 + $(LOCAL_RUN_ARGS)Add once near this target (outside this range):
LOCAL_RUN_ARGS ?= # Example: # make local-run LOCAL_RUN_ARGS='--unsupported-addon-features=TrustManager=true --v=4'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 318 - 319, The commented debug flags are currently hard-coded and require editing the Makefile; introduce a configurable variable (e.g., LOCAL_RUN_ARGS) and update the local-run target to append this variable when invoking the command so developers can pass flags at invocation time (for example: make local-run LOCAL_RUN_ARGS='--unsupported-addon-features=TrustManager=true --v=4'); add a default declaration like LOCAL_RUN_ARGS ?= near the top of the target and replace the commented flags in the local-run invocation with $(LOCAL_RUN_ARGS) so toggling is possible without file edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Around line 318-319: The commented debug flags are currently hard-coded and
require editing the Makefile; introduce a configurable variable (e.g.,
LOCAL_RUN_ARGS) and update the local-run target to append this variable when
invoking the command so developers can pass flags at invocation time (for
example: make local-run
LOCAL_RUN_ARGS='--unsupported-addon-features=TrustManager=true --v=4'); add a
default declaration like LOCAL_RUN_ARGS ?= near the top of the target and
replace the commented flags in the local-run invocation with $(LOCAL_RUN_ARGS)
so toggling is possible without file edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ea7a87d-3bd2-4897-a939-01bdee005e40
📒 Files selected for processing (3)
Makefilebundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/manager/manager.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- config/manager/manager.yaml
- bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
chiragkyal
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request references CM-867 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@chiragkyal: This pull request references CM-867 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@chiragkyal: This pull request references CM-867 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Member
Author
Member
Author
|
/retest |
|
@chiragkyal: An error was encountered searching for bug CM-867 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
request failed. Please analyze the request body for more details. Status code: 403:
Please contact an administrator to resolve this issue, then request a bug refresh with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/valid-reference
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/trustmanager/deployments_test.go`:
- Around line 475-477: The test is flaky because when tt.setImage is false it
leaves the environment variable uncontrolled; modify the test to explicitly
clear the env var in that branch so the "missing image" case is
deterministic—replace the current conditional with: when tt.setImage is true
call t.Setenv(trustManagerImageNameEnvVarName, testImage), otherwise call
t.Setenv(trustManagerImageNameEnvVarName, "") (or explicitly unset via
os.Unsetenv with proper cleanup) so trustManagerImageNameEnvVarName is
guaranteed absent/empty during the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8d1a114-b2d3-4129-a4f6-fc6552195a93
📒 Files selected for processing (9)
pkg/controller/trustmanager/certificates_test.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/rbacs_test.gopkg/controller/trustmanager/serviceaccounts_test.gopkg/controller/trustmanager/services_test.gopkg/controller/trustmanager/test_utils.gopkg/controller/trustmanager/utils_test.gopkg/controller/trustmanager/webhooks_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/e2e/trustmanager_test.go (2)
69-83:⚠️ Potential issue | 🟠 MajorMissing cleanup: Restore feature gate state after test suite.
The
BeforeAllmodifies the cluster-scoped Subscription to enableTrustManager=true, but there's no correspondingAfterAllto restore the originalUNSUPPORTED_ADDON_FEATURESvalue. This leaks state into subsequent e2e specs.Consider capturing the original value before patching and restoring it in
AfterAll:var originalUnsupportedAddonFeatures string BeforeAll(func() { // Capture original value first // ... then patch }) AfterAll(func() { // Restore original value using patchSubscriptionWithEnvVars })🤖 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 69 - 83, Capture the current value of UNSUPPORTED_ADDON_FEATURES into a package-level variable (e.g., originalUnsupportedAddonFeatures) inside BeforeAll before calling patchSubscriptionWithEnvVars, then add an AfterAll that calls patchSubscriptionWithEnvVars to restore the original value and uses waitForDeploymentEnvVarAndRollout to ensure the operator rolls out with the restored env var; reference the existing BeforeAll, patchSubscriptionWithEnvVars, waitForDeploymentEnvVarAndRollout and add an AfterAll to perform the restore and verification.
812-815:⚠️ Potential issue | 🟡 MinorReturn error on
Degraded=Truefor faster failure diagnosis.When the controller reports
Degraded=True, continuing to poll until timeout delays CI feedback. Returning an error with the degraded reason would help diagnose failures faster.degradedCondition := meta.FindStatusCondition(trustManagerStatus.Conditions, v1alpha1.Degraded) if degradedCondition != nil && degradedCondition.Status == metav1.ConditionTrue { - return false, nil + return false, fmt.Errorf("TrustManager is degraded: %s - %s", degradedCondition.Reason, degradedCondition.Message) }🤖 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 812 - 815, The test currently treats a Degraded=True condition as a non-fatal state; change the logic so that when meta.FindStatusCondition(trustManagerStatus.Conditions, v1alpha1.Degraded) returns a condition with Status==metav1.ConditionTrue the function returns an error instead of (false, nil). Construct the error including the degraded condition's Reason and/or Message (e.g., via fmt.Errorf("controller degraded: %s - %s", degradedCondition.Reason, degradedCondition.Message)) so callers fail fast and get diagnostic info; update the code path using degradedCondition, meta.FindStatusCondition, trustManagerStatus.Conditions and v1alpha1.Degraded accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/e2e/trustmanager_test.go`:
- Around line 69-83: Capture the current value of UNSUPPORTED_ADDON_FEATURES
into a package-level variable (e.g., originalUnsupportedAddonFeatures) inside
BeforeAll before calling patchSubscriptionWithEnvVars, then add an AfterAll that
calls patchSubscriptionWithEnvVars to restore the original value and uses
waitForDeploymentEnvVarAndRollout to ensure the operator rolls out with the
restored env var; reference the existing BeforeAll,
patchSubscriptionWithEnvVars, waitForDeploymentEnvVarAndRollout and add an
AfterAll to perform the restore and verification.
- Around line 812-815: The test currently treats a Degraded=True condition as a
non-fatal state; change the logic so that when
meta.FindStatusCondition(trustManagerStatus.Conditions, v1alpha1.Degraded)
returns a condition with Status==metav1.ConditionTrue the function returns an
error instead of (false, nil). Construct the error including the degraded
condition's Reason and/or Message (e.g., via fmt.Errorf("controller degraded: %s
- %s", degradedCondition.Reason, degradedCondition.Message)) so callers fail
fast and get diagnostic info; update the code path using degradedCondition,
meta.FindStatusCondition, trustManagerStatus.Conditions and v1alpha1.Degraded
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5fae2f5-dd94-4fbc-bacd-025ec1420a92
📒 Files selected for processing (2)
test/e2e/trustmanager_test.gotest/e2e/utils_test.go
|
@chiragkyal: An error was encountered searching for bug CM-867 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
request failed. Please analyze the request body for more details. Status code: 403:
Please contact an administrator to resolve this issue, then request a bug refresh with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
/retest |
| // certificateModified compares only the fields we manage via SSA. | ||
| // We compare individual spec fields rather than the full Spec because | ||
| // cert-manager's webhook may default fields we don't set (e.g. Duration). | ||
| func certificateModified(desired, existing *certmanagerv1.Certificate) bool { |
Contributor
There was a problem hiding this comment.
We should have had these *Modified() helpers in a common package and used for both istio-csr and trust-manager. If not possible now, we can pick it up for GA.
Member
Author
There was a problem hiding this comment.
I think we can do that during GA. I can create a tracker for that.
Contributor
There was a problem hiding this comment.
Yeah, for this and for configuring the issuer both, thank you!
Member
Author
There was a problem hiding this comment.
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
chiragkyal
force-pushed
the
trust-manager-reconcilers
branch
from
f6a97f2 to
de5b043
Compare
Contributor
|
/lgtm |
openshift-ci
bot
added
lgtm
Member
Author
|
/retest |
Member
Author
|
/retest |
2 similar comments
Member
Author
|
/retest |
Member
Author
|
/retest |
Signed-off-by: chiragkyal <ckyal@redhat.com>
Contributor
|
@chiragkyal: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Contributor
|
/lgtm Please refer link for the pre-merge validations. /label qe-approved Doc changes communicated as part of API PR, hence self adding the label /label docs-approved |
openshift-ci
bot
added
qe-approved
|
@chiragkyal: This pull request references CM-867 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh, chiragkyal The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 359b345
into
openshift:master
9 of 10 checks passed
arun717
pushed a commit
to arun717/cert-manager-operator
that referenced
this pull request
Mar 26, 2026
Add missing unit test coverage for configurable trust namespace feature. The trust namespace configuration was already fully implemented in PR openshift#379. This commit adds comprehensive unit tests to verify the implementation and satisfy CM-869 acceptance criteria. Tests added: - Trust namespace validation when namespace doesn't exist - Custom trust namespace configuration with successful reconciliation - Status field updates for both default and custom namespace values Without these tests, the trust namespace validation logic was not adequately covered, making it harder to catch regressions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-merge-bot bot
pushed a commit
that referenced
this pull request
Apr 2, 2026
* test(trustmanager): add unit tests for trust namespace configuration Add missing unit test coverage for configurable trust namespace feature. The trust namespace configuration was already fully implemented in PR #379. This commit adds comprehensive unit tests to verify the implementation and satisfy CM-869 acceptance criteria. Tests added: - Trust namespace validation when namespace doesn't exist - Custom trust namespace configuration with successful reconciliation - Status field updates for both default and custom namespace values Without these tests, the trust namespace validation logic was not adequately covered, making it harder to catch regressions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * test(e2e): add e2e tests for trust namespace configuration Add comprehensive end-to-end tests for trust namespace configuration. Tests verify: 1. Custom trust namespace RBAC placement - Role/RoleBinding created in trust namespace - Leader election Role/RoleBinding in operand namespace 2. Deployment configuration with correct --trust-namespace arg 3. Status field updates matching spec configuration 4. Degraded condition when trust namespace doesn't exist 5. Recovery to Ready=True after namespace creation These tests ensure the trust namespace feature works correctly in a real cluster environment and that all resources are created in the correct namespaces as required by CM-869. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * test(trustmanager): fix for missing namespace case. * Handling PR review comments. * Handling remaining PR review comments on trustmanager_test.go * Addressed Review comments iteration 3 * Handling CodeRabbit comment * Addressed new round of pr review comments * Undo comment for test * Addressing the latest review comment of removal of a test. * Removing duplicate method definition. --------- Co-authored-by: Arun Maurya <amaurya@amaurya-thinkpadp1gen7.bengluru.csb> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
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
5 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
reconcilers that create and maintain the lifecycle to run the trust-manager operand.
Changes
RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGERandTRUSTMANAGER_OPERAND_IMAGE_VERSIONenvironment variables to the CSV and manager deploymentmanifest.
--unsupported-addon-features="TrustManager=true"flag tolocal-runfor easy local testing.Implements