CM-902: Fix golangci-lint errors#368#372
Open
anandkuma77 wants to merge 1 commit intoopenshift:masterfrom
Open
CM-902: Fix golangci-lint errors#368#372anandkuma77 wants to merge 1 commit intoopenshift:masterfrom
anandkuma77 wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@anandkuma77: This pull request references CM-902 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. |
|
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:
WalkthroughThis PR refactors Kubernetes resource reconciliation logic by introducing shared reconciliation helpers in the trustmanager and istiocsr controllers to eliminate duplicated code. The changes consolidate the "check-exists → conditionally update if modified → emit events" pattern into reusable functions, reducing code duplication across RBAC, certificate, and webhook configuration resources. Additionally, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 6 | ❌ 4❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (6 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: anandkuma77 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 |
|
@anandkuma77: This pull request references CM-902 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
pkg/operator/starter.go (1)
45-67: Consider wrapping errors with context to aid debugging.Each error return in
initClientsloses context about which client failed to initialize. This makes troubleshooting harder when operators fail to start.♻️ Proposed fix to add error context
func initClients(cc *controllercmd.ControllerContext) (*kubernetes.Clientset, *certmanoperatorclient.Clientset, *apiextensionsclient.Clientset, *configv1client.Clientset, error) { kubeClient, err := kubernetes.NewForConfig(cc.ProtoKubeConfig) if err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, fmt.Errorf("failed to create kubernetes client: %w", err) } certManagerOperatorClient, err := certmanoperatorclient.NewForConfig(cc.KubeConfig) if err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, fmt.Errorf("failed to create cert-manager operator client: %w", err) } apiExtensionsClient, err := apiextensionsclient.NewForConfig(cc.KubeConfig) if err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, fmt.Errorf("failed to create api-extensions client: %w", err) } configClient, err := configv1client.NewForConfig(cc.KubeConfig) if err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, fmt.Errorf("failed to create config client: %w", err) } return kubeClient, certManagerOperatorClient, apiExtensionsClient, configClient, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/starter.go` around lines 45 - 67, The initClients function returns raw errors that lack context; update each error return after calls to kubernetes.NewForConfig, certmanoperatorclient.NewForConfig, apiextensionsclient.NewForConfig and configv1client.NewForConfig to wrap the error with a descriptive message (e.g., using fmt.Errorf("failed to create <client-name> client: %w", err) or errors.Wrapf) so logs show which client (kube, cert-manager operator, apiextensions, configv1) and which config (cc.ProtoKubeConfig vs cc.KubeConfig) failed to initialize.pkg/operator/optionalinformer/optional_informer_test.go (1)
103-109: Consider verifying the specific error witherrors.Is().Since you now have a sentinel error, you could make the test more precise by asserting the wrapped error:
♻️ Optional: More precise error assertion
t.Run("negative case with an expected error", func(t *testing.T) { errorProneDiscoveryClient := createErroneousFakeDiscoveryClient() _, err := NewOptionalInformer(context.TODO(), fixedGVRForTest, errorProneDiscoveryClient, dummyInformerInit) require.Error(t, err) + assert.True(t, errors.Is(err, errExpectedFoo), "expected error to wrap errExpectedFoo") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/optionalinformer/optional_informer_test.go` around lines 103 - 109, The test currently only checks that NewOptionalInformer (called with createErroneousFakeDiscoveryClient) returns an error; update it to assert the specific sentinel error is returned by using errors.Is(err, <sentinel error>) (import the standard errors package) — e.g. replace require.Error(t, err) with a require that errors.Is(err, <sentinel error>) is true to verify the wrapped sentinel error from NewOptionalInformer when createErroneousFakeDiscoveryClient triggers the failure.pkg/controller/deployment/deployment_overrides_validation.go (1)
122-129: Semantic mismatch:validateOverrideEnvreturnserrUnsupportedArginstead of a dedicated env error.The function validates environment variables but wraps errors with
errUnsupportedArg. For consistency and clarity, consider introducing a dedicatederrUnsupportedEnvsentinel error or renaming the existing one to be more generic (e.g.,errUnsupportedOverride).♻️ Option 1: Add dedicated error (preferred)
var ( errUnsupportedArg = errors.New("validation failed due to unsupported arg") + errUnsupportedEnv = errors.New("validation failed due to unsupported env") errUnsupportedLabel = errors.New("validation failed due to unsupported label") errUnsupportedResourceLimits = errors.New("validation failed due to unsupported resource limits") errUnsupportedResourceRequests = errors.New("validation failed due to unsupported resource requests") )Then update line 125:
- return fmt.Errorf("%q=%q: %w", k, v.Value, errUnsupportedArg) + return fmt.Errorf("%q=%q: %w", k, v.Value, errUnsupportedEnv)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/deployment/deployment_overrides_validation.go` around lines 122 - 129, validateOverrideEnv currently wraps unsupported env var errors with errUnsupportedArg which is semantically misleading; add a new sentinel error (e.g., errUnsupportedEnv) or rename errUnsupportedArg to a more generic name (e.g., errUnsupportedOverride), then update validateOverrideEnv to return the new sentinel (reference: function validateOverrideEnv and the error symbol errUnsupportedArg) so callers receive a dedicated env-specific error; ensure any other code that inspects errUnsupportedArg is updated to check the new sentinel or the renamed symbol to preserve behavior.pkg/operator/operatorclient/operatorclient.go (1)
238-255: Redundant nil checks onoperatorStatus.The variable
operatorStatusis initialized to a non-nil pointer on line 227 (&applyoperatorv1.OperatorStatusApplyConfiguration{}), so the nil checks on lines 240 and 250 will always evaluate to true.♻️ Suggested simplification
if desiredConfiguration.Conditions != nil { - var existingConditions []applyoperatorv1.OperatorConditionApplyConfiguration - if operatorStatus != nil { - existingConditions = operatorStatus.Conditions - } + existingConditions := operatorStatus.Conditions v1helpers.SetApplyConditionsLastTransitionTime(c.Clock, &desiredConfiguration.Conditions, existingConditions) } v1helpers.CanonicalizeOperatorStatus(desiredConfiguration) v1helpers.CanonicalizeOperatorStatus(operatorStatus) original := applyconfig.CertManager("cluster") - if operatorStatus != nil { - originalStatus := &applyconfig.CertManagerStatusApplyConfiguration{ - OperatorStatusApplyConfiguration: *operatorStatus, - } - original.WithStatus(originalStatus) + originalStatus := &applyconfig.CertManagerStatusApplyConfiguration{ + OperatorStatusApplyConfiguration: *operatorStatus, } + original.WithStatus(originalStatus)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/operatorclient/operatorclient.go` around lines 238 - 255, operatorStatus is already initialized to a non-nil &applyoperatorv1.OperatorStatusApplyConfiguration{}, so remove the redundant nil checks and simplify the logic: directly use operatorStatus when building existingConditions and when constructing originalStatus for original.WithStatus(...) (i.e., drop the if operatorStatus != nil guards around existingConditions assignment and originalStatus creation) and ensure v1helpers.SetApplyConditionsLastTransitionTime is still called with &desiredConfiguration.Conditions and existingConditions.pkg/operator/optionalinformer/optional_informer.go (1)
49-50: Pre-existing dead code: unused assignment.Line 49 computes
o.gvr.GroupVersion().String()and discards the result, then line 50 computes the same value again. This appears to be leftover from debugging or refactoring. Consider removing it in a follow-up.- _ = o.gvr.GroupVersion().String() resources, err := o.discoveryClient.ServerResourcesForGroupVersion(o.gvr.GroupVersion().String())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/optionalinformer/optional_informer.go` around lines 49 - 50, Remove the dead/unused assignment that calls o.gvr.GroupVersion().String() and discards its result; instead only call o.discoveryClient.ServerResourcesForGroupVersion(o.gvr.GroupVersion().String()) once (the existing call that assigns to resources, err) to avoid redundant computation and clear the unused expression in optional_informer.go around the OptionalInformer initialization code that references o.gvr.GroupVersion().String() and discoveryClient.ServerResourcesForGroupVersion.pkg/controller/deployment/cert_manager_networkpolicy.go (1)
221-248: Consider making the default case fail explicitly.Since
validateComponentNamealready validates that onlyCoreController,CAInjector, orWebhookare acceptable, this default case should be unreachable in normal flow. Silently returning a fallback selector could mask bugs if validation is bypassed in future refactors.♻️ Optional: Make the unreachable case explicit
func (c *CertManagerNetworkPolicyUserDefinedController) getPodSelectorForComponent(component v1alpha1.ComponentName) metav1.LabelSelector { switch component { case v1alpha1.CoreController: return metav1.LabelSelector{ MatchLabels: map[string]string{ "app": "cert-manager", }, } case v1alpha1.CAInjector: return metav1.LabelSelector{ MatchLabels: map[string]string{ "app": "cainjector", }, } case v1alpha1.Webhook: return metav1.LabelSelector{ MatchLabels: map[string]string{ "app": "webhook", }, } default: - return metav1.LabelSelector{ - MatchLabels: map[string]string{ - "app.kubernetes.io/name": "cert-manager", - }, - } + // This should be unreachable after validateComponentName check. + // Return a safe empty selector rather than silently selecting wrong pods. + return metav1.LabelSelector{} } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/deployment/cert_manager_networkpolicy.go` around lines 221 - 248, The default branch in getPodSelectorForComponent silently returns a fallback selector which can mask logic bugs; make the unreachable default fail explicitly (e.g., panic or log.Fatalf) with a clear message including the unexpected v1alpha1.ComponentName value so any bypass of validateComponentName is immediately obvious. Update the default case in getPodSelectorForComponent to produce a hard failure (including the offending component) rather than returning the fallback selector.pkg/controller/istiocsr/install_istiocsr.go (1)
10-19: Consider documenting label collision precedence explicitly.Current copy order makes
controllerDefaultResourceLabelswin over user-provided labels on key conflicts. A short comment on that intent would prevent ambiguity for future changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/install_istiocsr.go` around lines 10 - 19, The merge in setupResourceLabels currently copies user labels first then copies controllerDefaultResourceLabels, so controllerDefaultResourceLabels takes precedence on key conflicts; add a short inline comment above the merge (referencing setupResourceLabels, istiocsr.Spec.ControllerConfig.Labels and controllerDefaultResourceLabels) stating this explicit precedence intent and why the controller defaults must override user labels (or, if the desired behavior is the opposite, invert the copy order and document that user labels win). Ensure the comment clearly calls out that the final maps.Copy determines which side wins to avoid future ambiguity.pkg/controller/istiocsr/certificates.go (1)
92-109: Align RSA size validation with the declared allowed set.The runtime validation for RSA key sizes uses
size < DefaultRSAPrivateKeySize(which issize < 2048), allowing any size >= 2048. However, the API specification explicitly defines allowed RSA sizes as [2048, 4096, 8192] (see line 182 ofapi/operator/v1alpha1/istiocsr_types.go). This means unsupported sizes like 3072 or 5000 would incorrectly pass validation. The ECDSA validation correctly uses explicit equality checks for its allowed set. Tighten RSA validation to match the declared contract.🔧 Proposed tightening for private-key validation
func resolvePrivateKeyConfig(algorithm certmanagerv1.PrivateKeyAlgorithm, size int) (certmanagerv1.PrivateKeyAlgorithm, int, error) { if algorithm == "" { algorithm = DefaultPrivateKeyAlgorithm } - if size == 0 { - if algorithm == certmanagerv1.RSAKeyAlgorithm { - size = DefaultRSAPrivateKeySize - } - if algorithm == certmanagerv1.ECDSAKeyAlgorithm { - size = DefaultECDSA384PrivateKeySize - } - } - if (algorithm == certmanagerv1.RSAKeyAlgorithm && size < DefaultRSAPrivateKeySize) || - (algorithm == certmanagerv1.ECDSAKeyAlgorithm && size != DefaultECDSA256PrivateKeySize && size != DefaultECDSA384PrivateKeySize) { - return "", 0, errCertificateParamsNonCompliant - } - return algorithm, size, nil + switch algorithm { + case certmanagerv1.RSAKeyAlgorithm: + if size == 0 { + size = DefaultRSAPrivateKeySize + } + if size != 2048 && size != 4096 && size != 8192 { + return "", 0, errCertificateParamsNonCompliant + } + case certmanagerv1.ECDSAKeyAlgorithm: + if size == 0 { + size = DefaultECDSA384PrivateKeySize + } + if size != DefaultECDSA256PrivateKeySize && size != DefaultECDSA384PrivateKeySize { + return "", 0, errCertificateParamsNonCompliant + } + default: + return "", 0, errCertificateParamsNonCompliant + } + return algorithm, size, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/certificates.go` around lines 92 - 109, The RSA size check in resolvePrivateKeyConfig is too permissive (it currently allows any size >= DefaultRSAPrivateKeySize); change the RSA branch to only accept the explicit allowed RSA sizes (2048, 4096, 8192) — or the corresponding constants if defined — and otherwise return errCertificateParamsNonCompliant, mirroring how ECDSA sizes are validated; keep the function signature and error return (resolvePrivateKeyConfig, errCertificateParamsNonCompliant) unchanged.pkg/controller/istiocsr/deployments.go (1)
350-351: Redundant error wrapping creates confusing message.The error is wrapped twice: once inside
fmt.Errorfwith%wand again byNewIrrecoverableError. This produces a confusing error message. Since the key is already validated to not exist, the sentinel error alone with context inNewIrrecoverableErrorsuffices.♻️ Suggested simplification
- return NewIrrecoverableError(fmt.Errorf("key %q not found in ConfigMap %s/%s: %w", caCertConfig.Key, sourceConfigMapKey.Namespace, sourceConfigMapKey.Name, errConfigMapKeyNotFound), "invalid CA certificate ConfigMap %s/%s", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name) + return NewIrrecoverableError(errConfigMapKeyNotFound, "key %q not found in CA certificate ConfigMap %s/%s", caCertConfig.Key, sourceConfigMapKey.Namespace, sourceConfigMapKey.Name)🤖 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 350 - 351, The code double-wraps the sentinel error by embedding errConfigMapKeyNotFound inside fmt.Errorf with %w and then passing that into NewIrrecoverableError; remove the inner wrapping and pass the sentinel directly to NewIrrecoverableError, e.g. call NewIrrecoverableError(errConfigMapKeyNotFound, "invalid CA certificate ConfigMap %s/%s", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name) where the check uses sourceConfigMap.Data, caCertConfig.Key and sourceConfigMapKey.
🤖 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/deployment/credentials_request.go`:
- Line 117: The code calls
cloudCredentialsForPlatform(infra.Status.PlatformStatus.Type, secretName) but
dereferences infra.Status.PlatformStatus without checking for nil; update the
logic in the surrounding function to first ensure infra.Status is non-nil and
then use a conditional: if infra.Status.PlatformStatus != nil use
infra.Status.PlatformStatus.Type, otherwise fall back to infra.Status.Platform
(the non-nil field) and pass that value to cloudCredentialsForPlatform; ensure
you handle the case where infra or infra.Status is nil with an appropriate
error/early return.
In `@pkg/controller/istiocsr/controller.go`:
- Line 237: The call to r.log.Error is using a printf-style format string which
logr does not support; update the r.log.Error invocation (where
errInvalidLabelFormat, IstiocsrResourceWatchLabelName, value and obj.GetName()
are used) to pass a plain message string and then structured key/value pairs or
include the formatted error via fmt.Errorf before logging; e.g. replace the
printf-style second argument with a descriptive message and add "label",
IstiocsrResourceWatchLabelName, "value", value, "resource", obj.GetName() as
subsequent key/value args to r.log.Error.
In `@pkg/controller/istiocsr/deployments.go`:
- Around line 405-408: The call site using FromClientError for errors returned
by extractIssuerConfig is wrong because extractIssuerConfig returns
sentinel/internal errors (e.g., errFailedToConvertToClusterIssuer) rather than
client errors; change the error handling in the block that calls
extractIssuerConfig(obj, issuerRefKind) to return or wrap those sentinel errors
using the appropriate constructor (e.g., return
errFailedToConvertToClusterIssuer or use FromInternalError/WrapInternalError if
you have one) instead of FromClientError so the error type reflects the actual
failure from extractIssuerConfig.
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 270-278: The else branch currently executes whenever !(exist &&
hasObjectChanged) which also triggers when the resource does not exist; update
reconcileNamespacedRBACResource to only log "already exists and is in expected
state" when the resource truly exists and hasn't changed: either nest the
condition (if exist { if hasObjectChanged { ... } else { r.log.V(4).Info(...) }
} ) or change the else to an explicit check (else if exist && !hasObjectChanged)
so the unchanged-state log only runs when exist is true and hasObjectChanged is
false; refer to the symbols exist, hasObjectChanged,
reconcileNamespacedRBACResource, and r.log.V(4).Info(resourceDesc+" already
exists and is in expected state", "name", resourceName) when making the fix.
In `@pkg/controller/istiocsr/utils.go`:
- Around line 348-358: In containerProbeModified, guard against nil
ReadinessProbe and nil HTTPGet on both desired and fetched before accessing
HTTPGet.Path and other fields (compare existence first, then Path,
InitialDelaySeconds, PeriodSeconds), treating absent probes as unequal only when
the other has a probe; in containerResourcesModified, avoid direct dereference
of SecurityContext by first checking desired.SecurityContext and
fetched.SecurityContext for nil and comparing pointer presence and then fields
(or use reflect.DeepEqual on the pointers themselves) and ensure
VolumeMounts/Resources comparisons remain nil-safe; update these two functions
(containerProbeModified, containerResourcesModified) to perform these nil checks
and safe comparisons so no nil pointer dereference can occur.
---
Nitpick comments:
In `@pkg/controller/deployment/cert_manager_networkpolicy.go`:
- Around line 221-248: The default branch in getPodSelectorForComponent silently
returns a fallback selector which can mask logic bugs; make the unreachable
default fail explicitly (e.g., panic or log.Fatalf) with a clear message
including the unexpected v1alpha1.ComponentName value so any bypass of
validateComponentName is immediately obvious. Update the default case in
getPodSelectorForComponent to produce a hard failure (including the offending
component) rather than returning the fallback selector.
In `@pkg/controller/deployment/deployment_overrides_validation.go`:
- Around line 122-129: validateOverrideEnv currently wraps unsupported env var
errors with errUnsupportedArg which is semantically misleading; add a new
sentinel error (e.g., errUnsupportedEnv) or rename errUnsupportedArg to a more
generic name (e.g., errUnsupportedOverride), then update validateOverrideEnv to
return the new sentinel (reference: function validateOverrideEnv and the error
symbol errUnsupportedArg) so callers receive a dedicated env-specific error;
ensure any other code that inspects errUnsupportedArg is updated to check the
new sentinel or the renamed symbol to preserve behavior.
In `@pkg/controller/istiocsr/certificates.go`:
- Around line 92-109: The RSA size check in resolvePrivateKeyConfig is too
permissive (it currently allows any size >= DefaultRSAPrivateKeySize); change
the RSA branch to only accept the explicit allowed RSA sizes (2048, 4096, 8192)
— or the corresponding constants if defined — and otherwise return
errCertificateParamsNonCompliant, mirroring how ECDSA sizes are validated; keep
the function signature and error return (resolvePrivateKeyConfig,
errCertificateParamsNonCompliant) unchanged.
In `@pkg/controller/istiocsr/deployments.go`:
- Around line 350-351: The code double-wraps the sentinel error by embedding
errConfigMapKeyNotFound inside fmt.Errorf with %w and then passing that into
NewIrrecoverableError; remove the inner wrapping and pass the sentinel directly
to NewIrrecoverableError, e.g. call
NewIrrecoverableError(errConfigMapKeyNotFound, "invalid CA certificate ConfigMap
%s/%s", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name) where the check
uses sourceConfigMap.Data, caCertConfig.Key and sourceConfigMapKey.
In `@pkg/controller/istiocsr/install_istiocsr.go`:
- Around line 10-19: The merge in setupResourceLabels currently copies user
labels first then copies controllerDefaultResourceLabels, so
controllerDefaultResourceLabels takes precedence on key conflicts; add a short
inline comment above the merge (referencing setupResourceLabels,
istiocsr.Spec.ControllerConfig.Labels and controllerDefaultResourceLabels)
stating this explicit precedence intent and why the controller defaults must
override user labels (or, if the desired behavior is the opposite, invert the
copy order and document that user labels win). Ensure the comment clearly calls
out that the final maps.Copy determines which side wins to avoid future
ambiguity.
In `@pkg/operator/operatorclient/operatorclient.go`:
- Around line 238-255: operatorStatus is already initialized to a non-nil
&applyoperatorv1.OperatorStatusApplyConfiguration{}, so remove the redundant nil
checks and simplify the logic: directly use operatorStatus when building
existingConditions and when constructing originalStatus for
original.WithStatus(...) (i.e., drop the if operatorStatus != nil guards around
existingConditions assignment and originalStatus creation) and ensure
v1helpers.SetApplyConditionsLastTransitionTime is still called with
&desiredConfiguration.Conditions and existingConditions.
In `@pkg/operator/optionalinformer/optional_informer_test.go`:
- Around line 103-109: The test currently only checks that NewOptionalInformer
(called with createErroneousFakeDiscoveryClient) returns an error; update it to
assert the specific sentinel error is returned by using errors.Is(err, <sentinel
error>) (import the standard errors package) — e.g. replace require.Error(t,
err) with a require that errors.Is(err, <sentinel error>) is true to verify the
wrapped sentinel error from NewOptionalInformer when
createErroneousFakeDiscoveryClient triggers the failure.
In `@pkg/operator/optionalinformer/optional_informer.go`:
- Around line 49-50: Remove the dead/unused assignment that calls
o.gvr.GroupVersion().String() and discards its result; instead only call
o.discoveryClient.ServerResourcesForGroupVersion(o.gvr.GroupVersion().String())
once (the existing call that assigns to resources, err) to avoid redundant
computation and clear the unused expression in optional_informer.go around the
OptionalInformer initialization code that references
o.gvr.GroupVersion().String() and
discoveryClient.ServerResourcesForGroupVersion.
In `@pkg/operator/starter.go`:
- Around line 45-67: The initClients function returns raw errors that lack
context; update each error return after calls to kubernetes.NewForConfig,
certmanoperatorclient.NewForConfig, apiextensionsclient.NewForConfig and
configv1client.NewForConfig to wrap the error with a descriptive message (e.g.,
using fmt.Errorf("failed to create <client-name> client: %w", err) or
errors.Wrapf) so logs show which client (kube, cert-manager operator,
apiextensions, configv1) and which config (cc.ProtoKubeConfig vs cc.KubeConfig)
failed to initialize.
ℹ️ 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)
.golangci.yamlmain.gopkg/controller/deployment/cert_manager_networkpolicy.gopkg/controller/deployment/certmanager_controller.gopkg/controller/deployment/credentials_request.gopkg/controller/deployment/deployment_helper.gopkg/controller/deployment/deployment_overrides.gopkg/controller/deployment/deployment_overrides_validation.gopkg/controller/deployment/generic_deployment_controller.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/client.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_test.gopkg/controller/istiocsr/services_test.gopkg/controller/istiocsr/utils.gopkg/operator/operatorclient/operatorclient.gopkg/operator/optionalinformer/optional_informer.gopkg/operator/optionalinformer/optional_informer_test.gopkg/operator/starter.go
| return err | ||
| } | ||
|
|
||
| config, err := cloudCredentialsForPlatform(infra.Status.PlatformStatus.Type, secretName) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pkg/controller/deployment/credentials_request.go | head -150 | tail -50Repository: openshift/cert-manager-operator
Length of output: 2023
🏁 Script executed:
# Also check the struct definitions to understand PlatformStatus and Platform fields
rg -n "type.*Status\s*struct|PlatformStatus|Platform\s*" pkg/controller/deployment/credentials_request.go -A 3 -B 1Repository: openshift/cert-manager-operator
Length of output: 722
🏁 Script executed:
# Search for the struct definition in the infrastructure package
find . -name "*.go" -type f | xargs rg "type.*Status\s*struct" -l | head -5Repository: openshift/cert-manager-operator
Length of output: 490
🏁 Script executed:
# Find Infrastructure struct definition (from configv1)
rg -n "type Infrastructure struct" --type=goRepository: openshift/cert-manager-operator
Length of output: 172
🏁 Script executed:
# Look for PlatformStatus field definition
rg -n "PlatformStatus\s*\*?" --type=go -A 1 -B 1 | head -50Repository: openshift/cert-manager-operator
Length of output: 4293
🏁 Script executed:
# Check if there are nil guards for PlatformStatus elsewhere in the codebase
rg -n "PlatformStatus\s*!=\s*nil|PlatformStatus\s*==\s*nil" --type=goRepository: openshift/cert-manager-operator
Length of output: 177
🏁 Script executed:
# Check InfrastructureStatus struct to see if Platform field exists
rg -n "type InfrastructureStatus struct" --type=go -A 20Repository: openshift/cert-manager-operator
Length of output: 2465
🏁 Script executed:
# Look for Platform field definition (not PlatformStatus)
rg -n "^\s*Platform\s+" vendor/github.com/openshift/api/config/v1/types_infrastructure.go -B 2 -A 2Repository: openshift/cert-manager-operator
Length of output: 275
Add nil check before dereferencing infra.Status.PlatformStatus.Type to prevent panic.
Line 117 dereferences PlatformStatus without guarding against nil. Per OpenShift API design, PlatformStatus is an optional field and can be nil. When unset, this causes a panic. Use the non-nil Platform field as a fallback:
Suggested fix
- config, err := cloudCredentialsForPlatform(infra.Status.PlatformStatus.Type, secretName)
+ platformType := infra.Status.Platform
+ if infra.Status.PlatformStatus != nil {
+ platformType = infra.Status.PlatformStatus.Type
+ }
+
+ config, err := cloudCredentialsForPlatform(platformType, secretName)
if err != nil {
return 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
| config, err := cloudCredentialsForPlatform(infra.Status.PlatformStatus.Type, secretName) | |
| platformType := infra.Status.Platform | |
| if infra.Status.PlatformStatus != nil { | |
| platformType = infra.Status.PlatformStatus.Type | |
| } | |
| config, err := cloudCredentialsForPlatform(platformType, secretName) | |
| if err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/deployment/credentials_request.go` at line 117, The code calls
cloudCredentialsForPlatform(infra.Status.PlatformStatus.Type, secretName) but
dereferences infra.Status.PlatformStatus without checking for nil; update the
logic in the surrounding function to first ensure infra.Status is non-nil and
then use a conditional: if infra.Status.PlatformStatus != nil use
infra.Status.PlatformStatus.Type, otherwise fall back to infra.Status.Platform
(the non-nil field) and pass that value to cloudCredentialsForPlatform; ensure
you handle the case where infra or infra.Status is nil with an appropriate
error/early return.
|
@anandkuma77: This pull request references CM-902 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. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/istiocsr/rbacs.go (1)
147-158:⚠️ Potential issue | 🟡 MinorDo not silently persist empty RBAC names into status.
On Line 153 and Line 247, failure to resolve a resource name is only logged, then status is still updated. This can leave
Status.ClusterRole/Status.ClusterRoleBindingempty and hide a real reconciliation failure.🔧 Proposed fix
func (r *Reconciler) updateClusterRoleNameInStatus(istiocsr *v1alpha1.IstioCSR, desired, existing *rbacv1.ClusterRole) (string, error) { name := desired.GetName() if name == "" { if existing != nil && existing.GetName() != "" { name = existing.GetName() } else { - r.log.Error(errUpdateClusterRoleNameInStatus, "istiocsr", istiocsr.GetNamespace()) + return "", fmt.Errorf("%w: %s/%s", errUpdateClusterRoleNameInStatus, istiocsr.GetNamespace(), istiocsr.GetName()) } } istiocsr.Status.ClusterRole = name return name, r.updateStatus(r.ctx, istiocsr) } func (r *Reconciler) updateClusterRoleBindingNameInStatus(istiocsr *v1alpha1.IstioCSR, desired, existing *rbacv1.ClusterRoleBinding) error { name := desired.GetName() if name == "" { if existing != nil && existing.GetName() != "" { name = existing.GetName() } else { - r.log.Error(errUpdateClusterRoleBindingNameInStatus, "istiocsr", istiocsr.GetNamespace()) + return fmt.Errorf("%w: %s/%s", errUpdateClusterRoleBindingNameInStatus, istiocsr.GetNamespace(), istiocsr.GetName()) } } istiocsr.Status.ClusterRoleBinding = name return r.updateStatus(r.ctx, istiocsr) }Also applies to: 241-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/rbacs.go` around lines 147 - 158, The function updateClusterRoleNameInStatus currently logs when it cannot determine a ClusterRole name but proceeds to persist an empty Status.ClusterRole; change this so that when name is empty after checking desired and existing (i.e., failing to resolve the resource name) the function returns an error instead of setting istiocsr.Status.ClusterRole and calling updateStatus; apply the same change to the equivalent ClusterRoleBinding updater (the method handling ClusterRoleBinding name resolution around lines 241-252), ensuring both functions leave status unchanged and propagate an error when the resolved name is empty.
🤖 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/deployment/credentials_request.go`:
- Around line 130-142: The code assumes
deployment.Spec.Template.Spec.Containers[0] exists causing a panic on malformed
Deployments; update the reconciler to guard all accesses to Containers[0] by
checking len(deployment.Spec.Template.Spec.Containers) > 0 before appending
VolumeMounts or Env and if the slice is empty return a controlled error (or add
a default container) so functions that modify
deployment.Spec.Template.Spec.Containers[0] (the block that appends
config.volume, config.volumeMount and *config.envVar) never index into an empty
slice; ensure the same guard is used for where you append to
Containers[0].VolumeMounts and Containers[0].Env and include a clear reconciler
error message when returning.
- Around line 116-117: Replace the dynamic error returned in the infra == nil
path with a package-level sentinel error (similar to
errUnsupportedCloudProvider); declare a var like errInfrastructureNotFound =
errors.New("infrastructure \"cluster\" not found") at package scope in
pkg/controller/deployment/credentials_request.go and change the return in the
infra == nil branch to return errInfrastructureNotFound so the err113 linter is
satisfied.
In `@pkg/controller/istiocsr/utils.go`:
- Around line 530-534: The function shouldRejectInstance currently skips any
item in the same namespace (item.GetNamespace() == istiocsr.Namespace), which
allows multiple IstioCSR objects in one namespace to bypass rejection; change
the check to skip only the same resource instance by comparing identity—e.g.,
compare UIDs (item.GetUID() == istiocsr.UID) or both Name and Namespace
(item.GetName() == istiocsr.Name && item.GetNamespace() == istiocsr.Namespace)
so only the current object is ignored and other instances in the same namespace
are evaluated.
- Around line 382-383: The conditional that directly dereferences
desired.Spec.Replicas and fetched.Spec.Replicas can panic if either pointer is
nil; change the check to safely handle nils by first comparing nil-ness and then
values (e.g., treat nil vs non-nil as different, and only dereference when
non-nil) or extract the logic into a small helper like
replicasEqual(desired.Spec.Replicas, fetched.Spec.Replicas) used in the existing
if; update the if that currently reads using *desired.Spec.Replicas and
*fetched.Spec.Replicas to call this nil-safe comparison before evaluating the
rest of the expression involving Selector.MatchLabels.
---
Outside diff comments:
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 147-158: The function updateClusterRoleNameInStatus currently logs
when it cannot determine a ClusterRole name but proceeds to persist an empty
Status.ClusterRole; change this so that when name is empty after checking
desired and existing (i.e., failing to resolve the resource name) the function
returns an error instead of setting istiocsr.Status.ClusterRole and calling
updateStatus; apply the same change to the equivalent ClusterRoleBinding updater
(the method handling ClusterRoleBinding name resolution around lines 241-252),
ensuring both functions leave status unchanged and propagate an error when the
resolved name is empty.
ℹ️ 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 (5)
pkg/controller/deployment/credentials_request.gopkg/controller/istiocsr/controller.gopkg/controller/istiocsr/deployments.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/utils.go
Comment on lines
+130
to
+142
| deployment.Spec.Template.Spec.Volumes = append( | ||
| deployment.Spec.Template.Spec.Volumes, | ||
| *volume, | ||
| config.volume, | ||
| ) | ||
| deployment.Spec.Template.Spec.Containers[0].VolumeMounts = append( | ||
| deployment.Spec.Template.Spec.Containers[0].VolumeMounts, | ||
| *volumeMount, | ||
| config.volumeMount, | ||
| ) | ||
|
|
||
| if envVar != nil { | ||
| if config.envVar != nil { | ||
| deployment.Spec.Template.Spec.Containers[0].Env = append( | ||
| deployment.Spec.Template.Spec.Containers[0].Env, | ||
| *envVar, | ||
| *config.envVar, |
There was a problem hiding this comment.
Guard Containers[0] access to avoid reconcile-time panic.
Line 134/140 assume at least one container exists. If a malformed/partially-built deployment is passed, this panics instead of returning a reconciler error.
Suggested fix
+ if len(deployment.Spec.Template.Spec.Containers) == 0 {
+ return fmt.Errorf("deployment %q has no containers", deployment.Name)
+ }
+
+ container := &deployment.Spec.Template.Spec.Containers[0]
+
deployment.Spec.Template.Spec.Volumes = append(
deployment.Spec.Template.Spec.Volumes,
config.volume,
)
- deployment.Spec.Template.Spec.Containers[0].VolumeMounts = append(
- deployment.Spec.Template.Spec.Containers[0].VolumeMounts,
+ container.VolumeMounts = append(
+ container.VolumeMounts,
config.volumeMount,
)
if config.envVar != nil {
- deployment.Spec.Template.Spec.Containers[0].Env = append(
- deployment.Spec.Template.Spec.Containers[0].Env,
+ container.Env = append(
+ container.Env,
*config.envVar,
)
}📝 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
| deployment.Spec.Template.Spec.Volumes = append( | |
| deployment.Spec.Template.Spec.Volumes, | |
| *volume, | |
| config.volume, | |
| ) | |
| deployment.Spec.Template.Spec.Containers[0].VolumeMounts = append( | |
| deployment.Spec.Template.Spec.Containers[0].VolumeMounts, | |
| *volumeMount, | |
| config.volumeMount, | |
| ) | |
| if envVar != nil { | |
| if config.envVar != nil { | |
| deployment.Spec.Template.Spec.Containers[0].Env = append( | |
| deployment.Spec.Template.Spec.Containers[0].Env, | |
| *envVar, | |
| *config.envVar, | |
| if len(deployment.Spec.Template.Spec.Containers) == 0 { | |
| return fmt.Errorf("deployment %q has no containers", deployment.Name) | |
| } | |
| container := &deployment.Spec.Template.Spec.Containers[0] | |
| deployment.Spec.Template.Spec.Volumes = append( | |
| deployment.Spec.Template.Spec.Volumes, | |
| config.volume, | |
| ) | |
| container.VolumeMounts = append( | |
| container.VolumeMounts, | |
| config.volumeMount, | |
| ) | |
| if config.envVar != nil { | |
| container.Env = append( | |
| container.Env, | |
| *config.envVar, | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/deployment/credentials_request.go` around lines 130 - 142, The
code assumes deployment.Spec.Template.Spec.Containers[0] exists causing a panic
on malformed Deployments; update the reconciler to guard all accesses to
Containers[0] by checking len(deployment.Spec.Template.Spec.Containers) > 0
before appending VolumeMounts or Env and if the slice is empty return a
controlled error (or add a default container) so functions that modify
deployment.Spec.Template.Spec.Containers[0] (the block that appends
config.volume, config.volumeMount and *config.envVar) never index into an empty
slice; ensure the same guard is used for where you append to
Containers[0].VolumeMounts and Containers[0].Env and include a clear reconciler
error message when returning.
Comment on lines
+382
to
+383
| if *desired.Spec.Replicas != *fetched.Spec.Replicas || | ||
| !reflect.DeepEqual(desired.Spec.Selector.MatchLabels, fetched.Spec.Selector.MatchLabels) { |
There was a problem hiding this comment.
Guard Spec.Replicas pointers before dereference.
Line 382 dereferences desired.Spec.Replicas and fetched.Spec.Replicas directly. If either is nil, reconciliation panics.
🔧 Proposed fix
- if *desired.Spec.Replicas != *fetched.Spec.Replicas ||
- !reflect.DeepEqual(desired.Spec.Selector.MatchLabels, fetched.Spec.Selector.MatchLabels) {
+ if (desired.Spec.Replicas == nil) != (fetched.Spec.Replicas == nil) {
+ return true
+ }
+ if desired.Spec.Replicas != nil && *desired.Spec.Replicas != *fetched.Spec.Replicas {
+ return true
+ }
+ if !reflect.DeepEqual(desired.Spec.Selector.MatchLabels, fetched.Spec.Selector.MatchLabels) {
return true
}📝 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 *desired.Spec.Replicas != *fetched.Spec.Replicas || | |
| !reflect.DeepEqual(desired.Spec.Selector.MatchLabels, fetched.Spec.Selector.MatchLabels) { | |
| if (desired.Spec.Replicas == nil) != (fetched.Spec.Replicas == nil) { | |
| return true | |
| } | |
| if desired.Spec.Replicas != nil && *desired.Spec.Replicas != *fetched.Spec.Replicas { | |
| return true | |
| } | |
| if !reflect.DeepEqual(desired.Spec.Selector.MatchLabels, fetched.Spec.Selector.MatchLabels) { | |
| return true | |
| } |
🤖 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 382 - 383, The conditional
that directly dereferences desired.Spec.Replicas and fetched.Spec.Replicas can
panic if either pointer is nil; change the check to safely handle nils by first
comparing nil-ness and then values (e.g., treat nil vs non-nil as different, and
only dereference when non-nil) or extract the logic into a small helper like
replicasEqual(desired.Spec.Replicas, fetched.Spec.Replicas) used in the existing
if; update the if that currently reads using *desired.Spec.Replicas and
*fetched.Spec.Replicas to call this nil-safe comparison before evaluating the
rest of the expression involving Selector.MatchLabels.
Comment on lines
+530
to
+534
| func shouldRejectInstance(istiocsr *v1alpha1.IstioCSR, items []v1alpha1.IstioCSR) bool { | ||
| for _, item := range items { | ||
| if item.GetNamespace() == istiocsr.Namespace { | ||
| continue | ||
| } |
There was a problem hiding this comment.
shouldRejectInstance skips too broadly and can allow multiple instances.
Line 532 currently skips every item in the same namespace, not just the current object. That means multiple IstioCSR resources in one namespace can bypass rejection entirely.
🔧 Proposed fix
func shouldRejectInstance(istiocsr *v1alpha1.IstioCSR, items []v1alpha1.IstioCSR) bool {
for _, item := range items {
- if item.GetNamespace() == istiocsr.Namespace {
+ if item.GetNamespace() == istiocsr.GetNamespace() && item.GetName() == istiocsr.GetName() {
continue
}
if item.CreationTimestamp.Time.Before(istiocsr.CreationTimestamp.Time) ||
item.CreationTimestamp.Time.Equal(istiocsr.CreationTimestamp.Time) {
return true
}
}
return false
}📝 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 shouldRejectInstance(istiocsr *v1alpha1.IstioCSR, items []v1alpha1.IstioCSR) bool { | |
| for _, item := range items { | |
| if item.GetNamespace() == istiocsr.Namespace { | |
| continue | |
| } | |
| func shouldRejectInstance(istiocsr *v1alpha1.IstioCSR, items []v1alpha1.IstioCSR) bool { | |
| for _, item := range items { | |
| if item.GetNamespace() == istiocsr.GetNamespace() && item.GetName() == istiocsr.GetName() { | |
| continue | |
| } | |
| if item.CreationTimestamp.Time.Before(istiocsr.CreationTimestamp.Time) || | |
| item.CreationTimestamp.Time.Equal(istiocsr.CreationTimestamp.Time) { | |
| return true | |
| } | |
| } | |
| return false | |
| } |
🤖 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 530 - 534, The function
shouldRejectInstance currently skips any item in the same namespace
(item.GetNamespace() == istiocsr.Namespace), which allows multiple IstioCSR
objects in one namespace to bypass rejection; change the check to skip only the
same resource instance by comparing identity—e.g., compare UIDs (item.GetUID()
== istiocsr.UID) or both Name and Namespace (item.GetName() == istiocsr.Name &&
item.GetNamespace() == istiocsr.Namespace) so only the current object is ignored
and other instances in the same namespace are evaluated.
anandkuma77
force-pushed
the
cm-902-golangci-lint-secondpart
branch
from
60bec5d to
da5ac4e
Compare
|
@anandkuma77: This pull request references CM-902 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. |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (9)
pkg/controller/deployment/credentials_request.go (2)
142-152:⚠️ Potential issue | 🟠 MajorGuard
Containers[0]access to avoid reconcile-time panic.Lines 142 and 148 assume at least one container exists. A malformed or partially-built deployment with no containers will cause a panic instead of returning a reconciler error.
Suggested fix
+ if len(deployment.Spec.Template.Spec.Containers) == 0 { + return fmt.Errorf("deployment %q has no containers", deployment.Name) + } + + container := &deployment.Spec.Template.Spec.Containers[0] + deployment.Spec.Template.Spec.Volumes = append( deployment.Spec.Template.Spec.Volumes, config.volume, ) - deployment.Spec.Template.Spec.Containers[0].VolumeMounts = append( - deployment.Spec.Template.Spec.Containers[0].VolumeMounts, + container.VolumeMounts = append( + container.VolumeMounts, config.volumeMount, ) if config.envVar != nil { - deployment.Spec.Template.Spec.Containers[0].Env = append( - deployment.Spec.Template.Spec.Containers[0].Env, + container.Env = append( + container.Env, *config.envVar, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/deployment/credentials_request.go` around lines 142 - 152, Protect the direct access to deployment.Spec.Template.Spec.Containers[0] by checking that len(deployment.Spec.Template.Spec.Containers) > 0 before using Containers[0]; if zero, return an error from the reconciler (or create a default container if that fits the contract) so we don't panic. Update the block that appends config.volumeMount and config.envVar to first container (references: deployment.Spec.Template.Spec.Containers[0], config.volumeMount, config.envVar) to perform the length check and handle the empty-containers case consistently (return error or create container) before appending.
128-131:⚠️ Potential issue | 🟠 MajorIncomplete fix: add fallback to
infra.Status.PlatformwhenPlatformStatusis nil.The nil check prevents a panic, which is good. However, when
PlatformStatusis nil,platformTyperemains empty andcloudCredentialsForPlatformwill returnerrUnsupportedCloudProvider. Per OpenShift API,infra.Status.Platformis the non-optional field that should be used as a fallback.Suggested fix
var platformType configv1.PlatformType - if infra.Status.PlatformStatus != nil { + if infra.Status.PlatformStatus != nil { platformType = infra.Status.PlatformStatus.Type + } else { + platformType = infra.Status.Platform }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/deployment/credentials_request.go` around lines 128 - 131, The current logic sets platformType only from infra.Status.PlatformStatus when non-nil, leaving platformType empty if PlatformStatus is nil and causing cloudCredentialsForPlatform to return errUnsupportedCloudProvider; update the code in credentials_request.go to fallback to infra.Status.Platform when PlatformStatus is nil by assigning platformType = infra.Status.Platform (or equivalent cast to configv1.PlatformType) before calling cloudCredentialsForPlatform, ensuring platformType is always populated; reference the platformType variable, infra.Status.PlatformStatus, infra.Status.Platform, and the cloudCredentialsForPlatform call to locate where to apply the fix.pkg/controller/istiocsr/controller.go (1)
248-251:⚠️ Potential issue | 🟡 MinorUse structured key/value logging here.
logr.Logger.Errordoes not printf-format the message argument, so the%splaceholders will be logged literally and the label/resource context is lost.🐛 Proposed fix
- r.log.Error(errInvalidLabelFormat, "%s label value(%s) not in expected format on %s resource", IstiocsrResourceWatchLabelName, value, obj.GetName()) + r.log.Error(errInvalidLabelFormat, "label value not in expected format", + "label", IstiocsrResourceWatchLabelName, + "value", value, + "resource", obj.GetName(), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/controller.go` around lines 248 - 251, The error log call after splitting the label value uses printf-style placeholders which logr ignores; update the r.log.Error invocation in the block following key := strings.Split(value, "_") (where len(key) != 2) to use structured key/value pairs instead of a formatted message—pass a short message string plus pairs like "label", IstiocsrResourceWatchLabelName, "value", value, "resource", obj.GetName() along with the errInvalidLabelFormat error so the label and resource context are recorded; keep the same conditional and return behavior.pkg/controller/istiocsr/rbacs.go (1)
293-300:⚠️ Potential issue | 🟡 MinorOnly log the unchanged-state branch when the object really exists.
The
elsestill runs for!exist, so the create path logsalready exists and is in expected statejust before callingCreate.Proposed fix
if exist && hasObjectChanged(desired, fetched) { r.log.V(1).Info(resourceKind+" has been modified, updating to desired state", "name", resourceName) if err := r.UpdateWithRetry(r.ctx, desired); err != nil { return common.FromClientError(err, "failed to update %s %s", resourceName, resourceKind) } r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "%s %s reconciled back to desired state", resourceKind, resourceName) - } else { + } else if exist { r.log.V(4).Info(resourceKind+" already exists and is in expected state", "name", resourceName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/rbacs.go` around lines 293 - 300, The unchanged-state log is currently executed even when the object doesn't exist; change the control flow so the "already exists and is in expected state" r.log.V(4).Info call runs only when exist is true by replacing the final else with else if exist { ... }; keep the existing UpdateWithRetry(desired) and hasObjectChanged(desired, fetched) logic intact and ensure the create path (when !exist) is not swallowed by that log branch—use else if exist for the r.log.V(4).Info call tied to hasObjectChanged and UpdateWithRetry in the rbacs.go reconciler.pkg/controller/istiocsr/deployments.go (1)
397-406:⚠️ Potential issue | 🟠 MajorDon't silently skip CA issuers with an empty
secretName.With the new
issuerConfig.CA != nil && issuerConfig.CA.SecretName != ""guard, a misconfigured CA issuer no longer surfaceserrIssuerCANotConfigured; reconciliation just proceeds without mounting a root CA. LetcreateCAConfigMapFromIssuerSecretvalidateSecretName, and usecommon.FromErrorhere so its local validation failures stay irrecoverable.Proposed fix
- if issuerConfig.CA != nil && issuerConfig.CA.SecretName != "" { + if issuerConfig.CA != nil { if err := r.createCAConfigMapFromIssuerSecret(istiocsr, issuerConfig, resourceLabels); err != nil { - return common.FromClientError(err, "failed to create CA ConfigMap") + return common.FromError(err, "failed to create CA ConfigMap") } shouldUpdateVolume = true }🤖 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 397 - 406, The code currently skips CA issuers when issuerConfig.CA is non-nil but SecretName is empty; change the guard to just if issuerConfig.CA != nil then call r.createCAConfigMapFromIssuerSecret(istiocsr, issuerConfig, resourceLabels) so that that function can validate SecretName and surface errIssuerCANotConfigured; on error return common.FromError(err, "failed to create CA ConfigMap") (so local validation failures are irrecoverable) and set shouldUpdateVolume = true; keep the existing branch that calls r.createCAConfigMapFromIstiodCertificate(istiocsr, resourceLabels) only when issuerConfig.CA == nil and leave its error conversion as common.FromClientError if appropriate.pkg/controller/istiocsr/utils.go (4)
497-501:⚠️ Potential issue | 🟠 MajorAdd nil guards for
ReadinessProbeandHTTPGetaccess.This function accesses
desired.ReadinessProbe.HTTPGet.Pathwithout checking ifReadinessProbeorHTTPGetis nil. If either container has a nil probe, this will panic.🔧 Proposed fix with nil guards
func containerReadinessProbeModified(desired, fetched corev1.Container) bool { + if desired.ReadinessProbe == nil || fetched.ReadinessProbe == nil { + return desired.ReadinessProbe != fetched.ReadinessProbe + } + if desired.ReadinessProbe.HTTPGet == nil || fetched.ReadinessProbe.HTTPGet == nil { + return desired.ReadinessProbe.HTTPGet != fetched.ReadinessProbe.HTTPGet + } return desired.ReadinessProbe.HTTPGet.Path != fetched.ReadinessProbe.HTTPGet.Path || desired.ReadinessProbe.InitialDelaySeconds != fetched.ReadinessProbe.InitialDelaySeconds || desired.ReadinessProbe.PeriodSeconds != fetched.ReadinessProbe.PeriodSeconds }🤖 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 497 - 501, The containerReadinessProbeModified function currently dereferences desired.ReadinessProbe.HTTPGet.Path (and similar fields) without nil checks; update containerReadinessProbeModified to first guard against nil ReadinessProbe and nil HTTPGet on both desired and fetched, treating a nil vs non-nil probe/HTTPGet as a modification, and only compare Path, InitialDelaySeconds, and PeriodSeconds when the respective probe and HTTPGet are non-nil on both sides; reference the function name containerReadinessProbeModified and the fields ReadinessProbe, HTTPGet.Path, InitialDelaySeconds, and PeriodSeconds when locating where to add these checks.
436-437:⚠️ Potential issue | 🟠 MajorGuard
Spec.Replicaspointers before dereference.Line 436 dereferences
desired.Spec.Replicasandfetched.Spec.Replicasdirectly. If either is nil, this will panic during reconciliation.🔧 Proposed fix
func deploymentSpecModified(desired, fetched *appsv1.Deployment) bool { // check just the fields which are set by the controller and set in static manifest, // as fields with default values end up in manifest and causes plain check to fail. - if *desired.Spec.Replicas != *fetched.Spec.Replicas || - !reflect.DeepEqual(desired.Spec.Selector.MatchLabels, fetched.Spec.Selector.MatchLabels) { + if !replicasEqual(desired.Spec.Replicas, fetched.Spec.Replicas) || + !reflect.DeepEqual(desired.Spec.Selector.MatchLabels, fetched.Spec.Selector.MatchLabels) { return true }Add a nil-safe helper:
func replicasEqual(a, b *int32) bool { if a == nil && b == nil { return true } if a == nil || b == nil { return false } return *a == *b }🤖 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 436 - 437, The condition dereferences desired.Spec.Replicas and fetched.Spec.Replicas directly which can panic if either is nil; add a nil-safe helper function replicasEqual(a, b *int32) bool that returns true when both nil, false when one is nil, and compares values otherwise, then replace the direct comparison in the reconciliation check (the if that currently uses *desired.Spec.Replicas != *fetched.Spec.Replicas) with replicasEqual(desired.Spec.Replicas, fetched.Spec.Replicas) while keeping the existing selector MatchLabels DeepEqual check.
503-507:⚠️ Potential issue | 🟠 MajorGuard
SecurityContextpointers before dereference.Line 505 dereferences
*desired.SecurityContextand*fetched.SecurityContextwithout nil checks. This will panic if either container has a nil security context.🔧 Proposed fix
func containerResourcesModified(desired, fetched corev1.Container) bool { + if desired.SecurityContext == nil || fetched.SecurityContext == nil { + return desired.SecurityContext != fetched.SecurityContext || + !reflect.DeepEqual(desired.Resources, fetched.Resources) || + !reflect.DeepEqual(desired.VolumeMounts, fetched.VolumeMounts) + } return !reflect.DeepEqual(desired.Resources, fetched.Resources) || !reflect.DeepEqual(*desired.SecurityContext, *fetched.SecurityContext) || !reflect.DeepEqual(desired.VolumeMounts, fetched.VolumeMounts) }🤖 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 503 - 507, The function containerResourcesModified is dereferencing desired.SecurityContext and fetched.SecurityContext and can panic if either is nil; change the SecurityContext comparison to be nil-safe by comparing the pointers or their values via reflect.DeepEqual without dereferencing (e.g., reflect.DeepEqual(desired.SecurityContext, fetched.SecurityContext)) or by explicitly checking nils before dereferencing; update the condition in containerResourcesModified accordingly so it safely returns true when the security contexts differ and avoids any direct *SecurityContext dereference when a pointer may be nil.
636-650:⚠️ Potential issue | 🟠 Major
isOldestInstanceskips all items in the same namespace, allowing multiple instances.Line 638 skips any item in the same namespace (
item.GetNamespace() == istiocsr.Namespace), not just the current object being evaluated. This means if two IstioCSR resources exist in the same namespace, both will returntruefrom this function and both will be allowed to proceed, defeating the singleton enforcement.The check should skip only the exact same object (by name and namespace), not all objects in the namespace.
🔧 Proposed fix
func isOldestInstance(istiocsr *v1alpha1.IstioCSR, items []v1alpha1.IstioCSR) bool { for _, item := range items { - if item.GetNamespace() == istiocsr.Namespace { + if item.GetNamespace() == istiocsr.GetNamespace() && item.GetName() == istiocsr.GetName() { continue } // Even when timestamps are equal we treat this instance as not the oldest.🤖 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 636 - 650, The function isOldestInstance incorrectly skips all items in the same namespace; change the early-skip to only ignore the identical object by comparing both name and namespace of the candidate and istiocsr (use item.GetName()/GetNamespace() vs istiocsr.Name/Namespace or GetName()/GetNamespace()), so only the exact same resource is continued over and other same-namespace instances are still considered when deciding oldest; update isOldestInstance to continue only when item name and namespace match the istiocsr being evaluated.
🧹 Nitpick comments (7)
pkg/operator/operatorclient/operatorclient.go (1)
238-255: Optional: remove redundant nil checks foroperatorStatus.
operatorStatusis always initialized on Line 227, so the checks on Line 240 and Line 250 are unnecessary noise.♻️ Suggested simplification
if desiredConfiguration.Conditions != nil { - var existingConditions []applyoperatorv1.OperatorConditionApplyConfiguration - if operatorStatus != nil { - existingConditions = operatorStatus.Conditions - } - v1helpers.SetApplyConditionsLastTransitionTime(c.Clock, &desiredConfiguration.Conditions, existingConditions) + v1helpers.SetApplyConditionsLastTransitionTime(c.Clock, &desiredConfiguration.Conditions, operatorStatus.Conditions) } v1helpers.CanonicalizeOperatorStatus(desiredConfiguration) v1helpers.CanonicalizeOperatorStatus(operatorStatus) original := applyconfig.CertManager("cluster") - if operatorStatus != nil { - originalStatus := &applyconfig.CertManagerStatusApplyConfiguration{ - OperatorStatusApplyConfiguration: *operatorStatus, - } - original.WithStatus(originalStatus) - } + originalStatus := &applyconfig.CertManagerStatusApplyConfiguration{ + OperatorStatusApplyConfiguration: *operatorStatus, + } + original.WithStatus(originalStatus)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/operatorclient/operatorclient.go` around lines 238 - 255, operatorStatus is already initialized earlier, so remove the redundant nil checks around its uses: drop the "if operatorStatus != nil" guard before assigning existingConditions and before creating originalStatus; directly set existingConditions = operatorStatus.Conditions (or use operatorStatus.Conditions when building existingConditions) and instantiate originalStatus := &applyconfig.CertManagerStatusApplyConfiguration{OperatorStatusApplyConfiguration: *operatorStatus} then call original.WithStatus(originalStatus). Keep the calls to v1helpers.SetApplyConditionsLastTransitionTime, v1helpers.CanonicalizeOperatorStatus, and original.WithStatus unchanged except for removing the unnecessary nil branches.pkg/operator/starter.go (2)
45-67: Consider wrapping errors with context for easier debugging.The function returns raw errors without indicating which client failed to initialize. This is inconsistent with the error wrapping pattern used elsewhere in this file (e.g., lines 103, 148, 175) and the PR objective of "standardizing error handling using static wrapped errors."
♻️ Suggested improvement for consistent error wrapping
func initClients(cc *controllercmd.ControllerContext) (*kubernetes.Clientset, *certmanoperatorclient.Clientset, *apiextensionsclient.Clientset, *configv1client.Clientset, error) { kubeClient, err := kubernetes.NewForConfig(cc.ProtoKubeConfig) if err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, fmt.Errorf("failed to create kubernetes client: %w", err) } certManagerOperatorClient, err := certmanoperatorclient.NewForConfig(cc.KubeConfig) if err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, fmt.Errorf("failed to create cert-manager operator client: %w", err) } apiExtensionsClient, err := apiextensionsclient.NewForConfig(cc.KubeConfig) if err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, fmt.Errorf("failed to create api-extensions client: %w", err) } configClient, err := configv1client.NewForConfig(cc.KubeConfig) if err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, fmt.Errorf("failed to create config client: %w", err) } return kubeClient, certManagerOperatorClient, apiExtensionsClient, configClient, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/starter.go` around lines 45 - 67, The initClients function returns raw errors from kubernetes.NewForConfig, certmanoperatorclient.NewForConfig, apiextensionsclient.NewForConfig and configv1client.NewForConfig which lack context; update each error return in initClients to wrap the underlying error with a descriptive message (e.g. using fmt.Errorf("creating kube client: %w", err)) so callers can see which client failed to initialize, keeping the same returned types and nils.
178-182: Goroutine error is logged but not surfaced to the operator.If
manager.Startfails (e.g., due to port binding issues or early startup failures), the error is only logged. The operator continues running without knowledge that operand controllers failed to start.This fire-and-forget pattern is common for background controllers, but consider whether a startup failure should cause the operator to exit or report degraded status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/starter.go` around lines 178 - 182, The current goroutine calls manager.Start(ctx) and only logs errors via ctrl.Log.Error, which hides startup failures; modify this to surface the error to the operator by capturing the return value of manager.Start and forwarding it (for example, send the error on a dedicated errCh or call cancel and return the error to the caller) so the process can exit or update degraded status instead of silently continuing; update the block around manager.Start, ctrl.Log.Error and ctx to propagate the error to the operator bootstrap flow.pkg/controller/trustmanager/controller.go (1)
127-129:cleanUpcurrently has a no-op boolean contract.
Right now it always returnsfalse, so the requeue branch never executes. Consider makingcleanUpside-effect-only until retry semantics are actually needed.♻️ Simplification diff
- if requeue := r.cleanUp(trustManager); requeue { - return ctrl.Result{RequeueAfter: defaultRequeueTime}, nil - } + r.cleanUp(trustManager)-func (r *Reconciler) cleanUp(trustManager *v1alpha1.TrustManager) bool { +func (r *Reconciler) cleanUp(trustManager *v1alpha1.TrustManager) { // TODO: For GA, handle cleaning up of resources created for installing trust-manager operand. // As per Non-Goals in the enhancement, removing the TrustManager CR will not remove the // trust-manager deployment or its associated resources. r.eventRecorder.Eventf(trustManager, corev1.EventTypeWarning, "RemoveDeployment", "%s trustmanager marked for deletion, remove all resources created for trustmanager deployment manually", trustManager.GetName()) - return false }Also applies to: 171-176
🤖 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 127 - 129, The cleanUp(trustManager) function currently returns a boolean but is a no-op boolean contract; change cleanUp to be side-effect-only by removing the bool return type and any boolean logic inside it, then update callers (the two places that do "if requeue := r.cleanUp(trustManager); requeue { return ctrl.Result{RequeueAfter: defaultRequeueTime}, nil }") to simply call r.cleanUp(trustManager) without checking a return value; ensure the function signature and all references (cleanUp and the caller blocks in controller.go) are updated accordingly and build/compile-tested.pkg/controller/deployment/cert_manager_networkpolicy.go (1)
179-186: Consider reversing error message order for clarity.The current format
"%s: %w"produces messages like"webhook: unsupported component name". Consider reversing to"unsupported component name: %s"(without wrapping) or using a clearer format for better readability.♻️ Optional: Improve error message readability
func (c *CertManagerNetworkPolicyUserDefinedController) validateComponentName(componentName v1alpha1.ComponentName) error { switch componentName { case v1alpha1.CoreController, v1alpha1.CAInjector, v1alpha1.Webhook: return nil default: - return fmt.Errorf("%s: %w", componentName, errUnsupportedComponentName) + return fmt.Errorf("%w: %s", errUnsupportedComponentName, componentName) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/deployment/cert_manager_networkpolicy.go` around lines 179 - 186, In validateComponentName, change the error formatting to put the descriptive text first and not wrap the sentinel error; replace return fmt.Errorf("%s: %w", componentName, errUnsupportedComponentName) with return fmt.Errorf("unsupported component name: %s", componentName) so messages read clearly (e.g., "unsupported component name: webhook") while keeping the same function and component name checks (v1alpha1.CoreController, v1alpha1.CAInjector, v1alpha1.Webhook).pkg/controller/deployment/generic_deployment_controller.go (1)
20-28: Remove unused parameters from function signature to simplify the API.The
targetVersion(line 21) andversionRecorder(line 28) parameters are unused innewGenericDeploymentControllerand are only passed through from wrapper functions. Removing them would also eliminate the unnecessarystatusimport. Note that this refactor would require updating the function signature and all three call sites (incert_manager_webhook_deployment.go,cert_manager_controller_deployment.go, andcert_manager_cainjector_deployment.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/deployment/generic_deployment_controller.go` around lines 20 - 28, Remove the unused parameters from newGenericDeploymentController: delete the placeholder targetVersion/underscore parameter in the second position and remove the trailing versionRecorder parameter of type status.VersionGetter from the function signature in newGenericDeploymentController, update its callers in cert_manager_webhook_deployment.go, cert_manager_controller_deployment.go, and cert_manager_cainjector_deployment.go to stop passing those arguments, and remove the now-unused status import; ensure you only change the function signature (newGenericDeploymentController) and all three call sites so types and argument order remain consistent elsewhere.pkg/controller/istiocsr/install_istiocsr.go (1)
11-18: Make reserved-label handling explicit.
controllerDefaultResourceLabelsis copied afterspec.controllerConfig.labels, so any colliding user key is silently overwritten. If labels likeappare reserved for the watch predicates inpkg/controller/istiocsr/controller.go, reject them explicitly; otherwise reverse the copy order so user-specified labels are not dropped. A small collision-case unit test would also lock the intended precedence down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/install_istiocsr.go` around lines 11 - 18, buildResourceLabels currently copies controllerDefaultResourceLabels over user labels which silently overwrites collisions; update buildResourceLabels to explicitly handle reserved keys (those used by the watch predicates in controller.go, e.g., "app" and any others listed there) by either rejecting/returning an error when istiocsr.Spec.ControllerConfig.Labels contains any reserved label or by reversing the copy order so user labels take precedence and then explicitly set the controller defaults for non-reserved keys; modify the function to check istiocsr.Spec.ControllerConfig.Labels for reserved keys and return/propagate an error (or implement the reversed-maps.Copy logic), and add a unit test that verifies collision behavior (user-provided vs default) for a reserved key to lock the chosen precedence.
🤖 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/ctrlutil/constants.go`:
- Around line 1-6: Remove the unused duplicate constant ManagedResourceLabelKey
from the ctrlutil package: delete the const declaration named
ManagedResourceLabelKey in ctrlutil, run build/tests to ensure no callers
reference it, and remove any now-unused imports; if instead this was intended as
a migration from the existing definition in common (the other
ManagedResourceLabelKey), document the migration plan and update all references
to import from ctrlutil (and deprecate the common symbol) before keeping the
duplicate.
In `@pkg/controller/ctrlutil/errors.go`:
- Around line 74-79: The conditional that classifies certain apierrors as
irrecoverable incorrectly includes apierrors.IsServiceUnavailable(err); remove
apierrors.IsServiceUnavailable(err) from the
IsUnauthorized/IsForbidden/IsInvalid/IsBadRequest check so that
ServiceUnavailable errors fall through to the NewRetryRequiredError path. In
other words, update the conditional around NewIrrecoverableError (the block
using apierrors.IsUnauthorized, apierrors.IsForbidden, apierrors.IsInvalid,
apierrors.IsBadRequest) to exclude apierrors.IsServiceUnavailable(err), ensuring
apierrors.IsServiceUnavailable errors are handled by NewRetryRequiredError
instead.
In `@pkg/controller/ctrlutil/utils.go`:
- Around line 19-30: HasObjectChanged currently only checks labels via
ObjectMetadataModified; update it to detect real drift by comparing full
metadata (labels, annotations, ownerReferences, finalizers) and the resource
body (spec/status) rather than just labels. Modify ObjectMetadataModified to
include GetAnnotations(), GetOwnerReferences() and GetFinalizers() in the
DeepEqual comparison, and in HasObjectChanged add a second check that
deep-compares the objects' runtime parts: create metadata-stripped copies of
desired and fetched (zero out ObjectMeta fields) and use reflect.DeepEqual on
those copies to detect spec-level changes; return true if either metadata or
stripped-object comparisons differ. Ensure you reference and change the
HasObjectChanged and ObjectMetadataModified functions so both metadata and spec
differences are caught.
In `@pkg/controller/deployment/deployment_overrides_validation.go`:
- Around line 23-28: The validateOverrideEnv function is returning
errUnsupportedArg for unsupported environment variables which conflates env
validation failures with CLI-arg failures; add a distinct sentinel error (e.g.
errUnsupportedEnv) alongside the existing
errUnsupportedArg/errUnsupportedLabel/etc., have validateOverrideEnv return
errUnsupportedEnv for env-specific unsupported cases, and update any
callers/aggregation logic that currently checks for errUnsupportedArg (including
the code around validateOverrideEnv and the similar checks at lines referenced
~122-126) to treat errUnsupportedEnv separately so messages remain accurate.
In `@pkg/controller/istiocsr/certificates.go`:
- Around line 93-109: The function resolvePrivateKeyConfig currently accepts any
non-empty algorithm value; update it to explicitly reject unsupported algorithms
by checking after applying the default: if algorithm is not
certmanagerv1.RSAKeyAlgorithm and not certmanagerv1.ECDSAKeyAlgorithm then
return errCertificateParamsNonCompliant. Keep the existing defaulting behavior
(DefaultPrivateKeyAlgorithm, DefaultRSAPrivateKeySize,
DefaultECDSA384PrivateKeySize) and the existing size validations for RSA and
ECDSA so callers of resolvePrivateKeyConfig (e.g., updateCertificateParams)
never receive an unsupported algorithm like "Ed25519".
In `@pkg/controller/istiocsr/controller.go`:
- Around line 88-107: The finalizer is being removed even though cleanUp() is a
no-op, which allows cluster-scoped RBAC and other managed resources created by
reconcileIstioCSRDeployment() (see install_istiocsr.go) to be orphaned; update
cleanUp(ctx, istiocsr) to actually delete all resources created by
reconcileIstioCSRDeployment (e.g., ClusterRole, ClusterRoleBinding,
ServiceAccount, any ClusterScoped objects) or call a new helper that
encapsulates that deletion, ensure cleanUp returns requeue=true if deletions are
in progress/errors, and only call removeFinalizer(ctx, istiocsr) after cleanUp
reports success so removeFinalizer is not invoked prematurely.
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 69-91: The helper resolveExistingClusterScopedResource currently
returns exist=false immediately if statusName is set and the named object is not
found, which prevents falling back to label-based resolution; change the logic
inside resolveExistingClusterScopedResource so that when statusName != "" you
still call listAndResolve() if the explicit fetch indicates the object is
missing (Exists returns false) or the client error is a NotFound—only treat
other fetch errors as failures (use common.FromClientError for those). In short:
keep using r.Exists(r.ctx, key, fetched) but if it returns false (or a NotFound
error) invoke listAndResolve() to locate an existing label-matched
ClusterRole/ClusterRoleBinding instead of returning false immediately.
In `@pkg/controller/istiocsr/utils.go`:
- Around line 652-660: The log call in rejectAlreadyMarkedInstance uses printf
placeholders ("%s/%s") which won't be substituted by logr; replace the
r.log.V(4).Info call to use structured key/value pairs instead (e.g., a plain
message like "istiocsr resource contains processing rejected annotation" and
keys "namespace", istiocsr.Namespace and "name", istiocsr.Name) so the Namespace
and Name appear correctly in logs; update the r.log.V(4).Info invocation
accordingly in the rejectAlreadyMarkedInstance function.
In `@pkg/controller/trustmanager/utils.go`:
- Around line 116-120: Change decodeServiceAccountObjBytes to return
(*corev1.ServiceAccount, error) instead of panicking: replace the panic branch
with a descriptive error (e.g., fmt.Errorf("decoded object is not a
ServiceAccount: got %T", obj)) and return (nil, err) on failure; on success
return (sa, nil). Update the call site in createOrApplyServiceAccounts (and its
caller) to accept the (sa, err) signature, check and propagate or handle the
error in the reconciliation path rather than letting it panic, and ensure any
logging or requeue behavior follows the operator's error-handling conventions.
---
Duplicate comments:
In `@pkg/controller/deployment/credentials_request.go`:
- Around line 142-152: Protect the direct access to
deployment.Spec.Template.Spec.Containers[0] by checking that
len(deployment.Spec.Template.Spec.Containers) > 0 before using Containers[0]; if
zero, return an error from the reconciler (or create a default container if that
fits the contract) so we don't panic. Update the block that appends
config.volumeMount and config.envVar to first container (references:
deployment.Spec.Template.Spec.Containers[0], config.volumeMount, config.envVar)
to perform the length check and handle the empty-containers case consistently
(return error or create container) before appending.
- Around line 128-131: The current logic sets platformType only from
infra.Status.PlatformStatus when non-nil, leaving platformType empty if
PlatformStatus is nil and causing cloudCredentialsForPlatform to return
errUnsupportedCloudProvider; update the code in credentials_request.go to
fallback to infra.Status.Platform when PlatformStatus is nil by assigning
platformType = infra.Status.Platform (or equivalent cast to
configv1.PlatformType) before calling cloudCredentialsForPlatform, ensuring
platformType is always populated; reference the platformType variable,
infra.Status.PlatformStatus, infra.Status.Platform, and the
cloudCredentialsForPlatform call to locate where to apply the fix.
In `@pkg/controller/istiocsr/controller.go`:
- Around line 248-251: The error log call after splitting the label value uses
printf-style placeholders which logr ignores; update the r.log.Error invocation
in the block following key := strings.Split(value, "_") (where len(key) != 2) to
use structured key/value pairs instead of a formatted message—pass a short
message string plus pairs like "label", IstiocsrResourceWatchLabelName, "value",
value, "resource", obj.GetName() along with the errInvalidLabelFormat error so
the label and resource context are recorded; keep the same conditional and
return behavior.
In `@pkg/controller/istiocsr/deployments.go`:
- Around line 397-406: The code currently skips CA issuers when issuerConfig.CA
is non-nil but SecretName is empty; change the guard to just if issuerConfig.CA
!= nil then call r.createCAConfigMapFromIssuerSecret(istiocsr, issuerConfig,
resourceLabels) so that that function can validate SecretName and surface
errIssuerCANotConfigured; on error return common.FromError(err, "failed to
create CA ConfigMap") (so local validation failures are irrecoverable) and set
shouldUpdateVolume = true; keep the existing branch that calls
r.createCAConfigMapFromIstiodCertificate(istiocsr, resourceLabels) only when
issuerConfig.CA == nil and leave its error conversion as common.FromClientError
if appropriate.
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 293-300: The unchanged-state log is currently executed even when
the object doesn't exist; change the control flow so the "already exists and is
in expected state" r.log.V(4).Info call runs only when exist is true by
replacing the final else with else if exist { ... }; keep the existing
UpdateWithRetry(desired) and hasObjectChanged(desired, fetched) logic intact and
ensure the create path (when !exist) is not swallowed by that log branch—use
else if exist for the r.log.V(4).Info call tied to hasObjectChanged and
UpdateWithRetry in the rbacs.go reconciler.
In `@pkg/controller/istiocsr/utils.go`:
- Around line 497-501: The containerReadinessProbeModified function currently
dereferences desired.ReadinessProbe.HTTPGet.Path (and similar fields) without
nil checks; update containerReadinessProbeModified to first guard against nil
ReadinessProbe and nil HTTPGet on both desired and fetched, treating a nil vs
non-nil probe/HTTPGet as a modification, and only compare Path,
InitialDelaySeconds, and PeriodSeconds when the respective probe and HTTPGet are
non-nil on both sides; reference the function name
containerReadinessProbeModified and the fields ReadinessProbe, HTTPGet.Path,
InitialDelaySeconds, and PeriodSeconds when locating where to add these checks.
- Around line 436-437: The condition dereferences desired.Spec.Replicas and
fetched.Spec.Replicas directly which can panic if either is nil; add a nil-safe
helper function replicasEqual(a, b *int32) bool that returns true when both nil,
false when one is nil, and compares values otherwise, then replace the direct
comparison in the reconciliation check (the if that currently uses
*desired.Spec.Replicas != *fetched.Spec.Replicas) with
replicasEqual(desired.Spec.Replicas, fetched.Spec.Replicas) while keeping the
existing selector MatchLabels DeepEqual check.
- Around line 503-507: The function containerResourcesModified is dereferencing
desired.SecurityContext and fetched.SecurityContext and can panic if either is
nil; change the SecurityContext comparison to be nil-safe by comparing the
pointers or their values via reflect.DeepEqual without dereferencing (e.g.,
reflect.DeepEqual(desired.SecurityContext, fetched.SecurityContext)) or by
explicitly checking nils before dereferencing; update the condition in
containerResourcesModified accordingly so it safely returns true when the
security contexts differ and avoids any direct *SecurityContext dereference when
a pointer may be nil.
- Around line 636-650: The function isOldestInstance incorrectly skips all items
in the same namespace; change the early-skip to only ignore the identical object
by comparing both name and namespace of the candidate and istiocsr (use
item.GetName()/GetNamespace() vs istiocsr.Name/Namespace or
GetName()/GetNamespace()), so only the exact same resource is continued over and
other same-namespace instances are still considered when deciding oldest; update
isOldestInstance to continue only when item name and namespace match the
istiocsr being evaluated.
---
Nitpick comments:
In `@pkg/controller/deployment/cert_manager_networkpolicy.go`:
- Around line 179-186: In validateComponentName, change the error formatting to
put the descriptive text first and not wrap the sentinel error; replace return
fmt.Errorf("%s: %w", componentName, errUnsupportedComponentName) with return
fmt.Errorf("unsupported component name: %s", componentName) so messages read
clearly (e.g., "unsupported component name: webhook") while keeping the same
function and component name checks (v1alpha1.CoreController,
v1alpha1.CAInjector, v1alpha1.Webhook).
In `@pkg/controller/deployment/generic_deployment_controller.go`:
- Around line 20-28: Remove the unused parameters from
newGenericDeploymentController: delete the placeholder targetVersion/underscore
parameter in the second position and remove the trailing versionRecorder
parameter of type status.VersionGetter from the function signature in
newGenericDeploymentController, update its callers in
cert_manager_webhook_deployment.go, cert_manager_controller_deployment.go, and
cert_manager_cainjector_deployment.go to stop passing those arguments, and
remove the now-unused status import; ensure you only change the function
signature (newGenericDeploymentController) and all three call sites so types and
argument order remain consistent elsewhere.
In `@pkg/controller/istiocsr/install_istiocsr.go`:
- Around line 11-18: buildResourceLabels currently copies
controllerDefaultResourceLabels over user labels which silently overwrites
collisions; update buildResourceLabels to explicitly handle reserved keys (those
used by the watch predicates in controller.go, e.g., "app" and any others listed
there) by either rejecting/returning an error when
istiocsr.Spec.ControllerConfig.Labels contains any reserved label or by
reversing the copy order so user labels take precedence and then explicitly set
the controller defaults for non-reserved keys; modify the function to check
istiocsr.Spec.ControllerConfig.Labels for reserved keys and return/propagate an
error (or implement the reversed-maps.Copy logic), and add a unit test that
verifies collision behavior (user-provided vs default) for a reserved key to
lock the chosen precedence.
In `@pkg/controller/trustmanager/controller.go`:
- Around line 127-129: The cleanUp(trustManager) function currently returns a
boolean but is a no-op boolean contract; change cleanUp to be side-effect-only
by removing the bool return type and any boolean logic inside it, then update
callers (the two places that do "if requeue := r.cleanUp(trustManager); requeue
{ return ctrl.Result{RequeueAfter: defaultRequeueTime}, nil }") to simply call
r.cleanUp(trustManager) without checking a return value; ensure the function
signature and all references (cleanUp and the caller blocks in controller.go)
are updated accordingly and build/compile-tested.
In `@pkg/operator/operatorclient/operatorclient.go`:
- Around line 238-255: operatorStatus is already initialized earlier, so remove
the redundant nil checks around its uses: drop the "if operatorStatus != nil"
guard before assigning existingConditions and before creating originalStatus;
directly set existingConditions = operatorStatus.Conditions (or use
operatorStatus.Conditions when building existingConditions) and instantiate
originalStatus :=
&applyconfig.CertManagerStatusApplyConfiguration{OperatorStatusApplyConfiguration:
*operatorStatus} then call original.WithStatus(originalStatus). Keep the calls
to v1helpers.SetApplyConditionsLastTransitionTime,
v1helpers.CanonicalizeOperatorStatus, and original.WithStatus unchanged except
for removing the unnecessary nil branches.
In `@pkg/operator/starter.go`:
- Around line 45-67: The initClients function returns raw errors from
kubernetes.NewForConfig, certmanoperatorclient.NewForConfig,
apiextensionsclient.NewForConfig and configv1client.NewForConfig which lack
context; update each error return in initClients to wrap the underlying error
with a descriptive message (e.g. using fmt.Errorf("creating kube client: %w",
err)) so callers can see which client failed to initialize, keeping the same
returned types and nils.
- Around line 178-182: The current goroutine calls manager.Start(ctx) and only
logs errors via ctrl.Log.Error, which hides startup failures; modify this to
surface the error to the operator by capturing the return value of manager.Start
and forwarding it (for example, send the error on a dedicated errCh or call
cancel and return the error to the caller) so the process can exit or update
degraded status instead of silently continuing; update the block around
manager.Start, ctrl.Log.Error and ctx to propagate the error to the operator
bootstrap flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 614f22a3-38e6-4be2-a008-ab1ee923c104
📒 Files selected for processing (43)
.golangci.yamlmain.gopkg/controller/common/client.gopkg/controller/common/constants.gopkg/controller/common/errors.gopkg/controller/common/reconcile_result.gopkg/controller/common/utils.gopkg/controller/ctrlutil/client.gopkg/controller/ctrlutil/constants.gopkg/controller/ctrlutil/errors.gopkg/controller/ctrlutil/fakes/fake_ctrl_client.gopkg/controller/ctrlutil/reconcile_result.gopkg/controller/ctrlutil/utils.gopkg/controller/deployment/cert_manager_networkpolicy.gopkg/controller/deployment/certmanager_controller.gopkg/controller/deployment/credentials_request.gopkg/controller/deployment/deployment_helper.gopkg/controller/deployment/deployment_overrides.gopkg/controller/deployment/deployment_overrides_validation.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_test.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/utils.gopkg/operator/operatorclient/operatorclient.gopkg/operator/optionalinformer/optional_informer.gopkg/operator/optionalinformer/optional_informer_test.gopkg/operator/setup_manager.gopkg/operator/starter.go
✅ Files skipped from review due to trivial changes (4)
- pkg/controller/trustmanager/constants.go
- pkg/operator/setup_manager.go
- pkg/controller/common/reconcile_result.go
- pkg/controller/common/utils.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/operator/optionalinformer/optional_informer_test.go
- pkg/controller/istiocsr/serviceaccounts_test.go
- pkg/controller/istiocsr/networkpolicies.go
- pkg/controller/deployment/certmanager_controller.go
- pkg/controller/deployment/deployment_helper.go
- pkg/controller/istiocsr/deployments_test.go
Comment on lines
+1
to
+6
| package ctrlutil | ||
|
|
||
| // ManagedResourceLabelKey is the common label key used by all operand controllers | ||
| // to identify resources they manage. Each controller uses a different value | ||
| // to distinguish its resources. | ||
| const ManagedResourceLabelKey = "app" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check usage of ManagedResourceLabelKey across the codebase
echo "=== Usage of ManagedResourceLabelKey ==="
rg -n "ManagedResourceLabelKey" --type go
echo ""
echo "=== Check imports of common vs ctrlutil packages ==="
rg -n "controller/common|controller/ctrlutil" --type go | head -50Repository: openshift/cert-manager-operator
Length of output: 4355
🏁 Script executed:
cat -n pkg/controller/ctrlutil/constants.goRepository: openshift/cert-manager-operator
Length of output: 365
🏁 Script executed:
# Check if there are other constants or code in ctrlutil that might explain the duplication
find pkg/controller/ctrlutil -type f -name "*.go" | head -20Repository: openshift/cert-manager-operator
Length of output: 306
🏁 Script executed:
# Check if ctrlutil/constants.go is the only file or if there's a broader refactoring happening
wc -l pkg/controller/ctrlutil/constants.go
rg -n "ctrlutil" --type go -A 2 -B 2 | head -60Repository: openshift/cert-manager-operator
Length of output: 1516
🏁 Script executed:
cat -n pkg/controller/common/constants.goRepository: openshift/cert-manager-operator
Length of output: 445
Remove unused duplicate constant or document migration plan.
ManagedResourceLabelKey = "app" is defined identically in both pkg/controller/common/constants.go and pkg/controller/ctrlutil/constants.go. The constant in ctrlutil is currently unused—all 12 references in the codebase import from common.
Either remove the duplicate from ctrlutil or, if this is part of a planned migration from common to ctrlutil, document the migration strategy and update references accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/ctrlutil/constants.go` around lines 1 - 6, Remove the unused
duplicate constant ManagedResourceLabelKey from the ctrlutil package: delete the
const declaration named ManagedResourceLabelKey in ctrlutil, run build/tests to
ensure no callers reference it, and remove any now-unused imports; if instead
this was intended as a migration from the existing definition in common (the
other ManagedResourceLabelKey), document the migration plan and update all
references to import from ctrlutil (and deprecate the common symbol) before
keeping the duplicate.
Comment on lines
+74
to
+79
| if apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err) || apierrors.IsInvalid(err) || | ||
| apierrors.IsBadRequest(err) || apierrors.IsServiceUnavailable(err) { | ||
| return NewIrrecoverableError(err, message, args...) | ||
| } | ||
|
|
||
| return NewRetryRequiredError(err, message, args...) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat pkg/controller/ctrlutil/errors.goRepository: openshift/cert-manager-operator
Length of output: 3449
🏁 Script executed:
rg "FromClientError" -A 3 -B 3Repository: openshift/cert-manager-operator
Length of output: 22359
🏁 Script executed:
cat pkg/controller/common/errors.goRepository: openshift/cert-manager-operator
Length of output: 3522
🌐 Web query:
Kubernetes apierrors IsServiceUnavailable 503 transient retryable
💡 Result:
apierrors.IsServiceUnavailable(err) is the Kubernetes/apimachinery helper that returns true when an error unwraps to a Kubernetes StatusError whose reason is “ServiceUnavailable” (typically surfaced as HTTP 503 from the apiserver / aggregated apis / webhooks). It’s a strong signal the failure is transient (the server/underlying service can’t handle the request right now), so retry with backoff is usually appropriate—especially for idempotent reads. [1]
Practical “retryable 503” guidance
- Safe to retry (typical):
GET,LIST,WATCH(and many PATCH/PUT/POST only if you have your own idempotency strategy). - Use server hints when present: check
apierrors.SuggestsClientDelay(err)(often driven byRetry-After) before retrying. [1] - Also consider retrying these related transient classes:
apierrors.IsServerTimeout(err)explicitly indicates the request “needs to be retried”. [1]
client-go behavior (important)
If you’re using client-go, it already has retry plumbing at the REST layer that:
- treats any 5xx (including 503) and 429 as candidates for waiting based on
Retry-After[2] - i.e., it can automatically honor server “wait N seconds then retry” guidance for 503 responses. [2]
Minimal Go pattern
import (
"context"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/util/wait"
"time"
)
err := wait.ExponentialBackoff(wait.Backoff{
Duration: 200 * time.Millisecond,
Factor: 2.0,
Jitter: 0.1,
Steps: 6,
}, func() (bool, error) {
_, err := client.CoreV1().Pods("ns").List(context.TODO(), metav1.ListOptions{})
if err == nil { return true, nil }
if errors.IsServiceUnavailable(err) || errors.IsServerTimeout(err) {
if d, ok := errors.SuggestsClientDelay(err); ok {
time.Sleep(time.Duration(d) * time.Second)
}
return false, nil // retry
}
return false, err // don't retry
})Rule of thumb: treat IsServiceUnavailable (503) as transient/retryable for reads, and for writes only retry if you can guarantee idempotency (otherwise you risk duplicate side effects). [1]
Sources: k8s.io/apimachinery errors package docs (IsServiceUnavailable / IsServerTimeout / SuggestsClientDelay) [1]; client-go REST retry code showing 5xx/429 use Retry-After waiting [2].
[1] https://pkg.go.dev/k8s.io/apimachinery/pkg/api/errors
[2] https://sources.debian.org/src/golang-k8s-client-go/0.32.3-2/rest/with_retry.go/
Move ServiceUnavailable errors to the retryable path.
A 503 response from the Kubernetes API server or an admission webhook indicates a transient failure—the server cannot handle the request right now and should recover on retry. Classifying apierrors.IsServiceUnavailable(err) as irrecoverable prevents requeues for failures that would heal automatically, causing unnecessary reconciliation failures.
Proposed fix
if apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err) || apierrors.IsInvalid(err) ||
- apierrors.IsBadRequest(err) || apierrors.IsServiceUnavailable(err) {
+ apierrors.IsBadRequest(err) {
return NewIrrecoverableError(err, message, args...)
}
return NewRetryRequiredError(err, message, args...)📝 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 apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err) || apierrors.IsInvalid(err) || | |
| apierrors.IsBadRequest(err) || apierrors.IsServiceUnavailable(err) { | |
| return NewIrrecoverableError(err, message, args...) | |
| } | |
| return NewRetryRequiredError(err, message, args...) | |
| 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/ctrlutil/errors.go` around lines 74 - 79, The conditional that
classifies certain apierrors as irrecoverable incorrectly includes
apierrors.IsServiceUnavailable(err); remove apierrors.IsServiceUnavailable(err)
from the IsUnauthorized/IsForbidden/IsInvalid/IsBadRequest check so that
ServiceUnavailable errors fall through to the NewRetryRequiredError path. In
other words, update the conditional around NewIrrecoverableError (the block
using apierrors.IsUnauthorized, apierrors.IsForbidden, apierrors.IsInvalid,
apierrors.IsBadRequest) to exclude apierrors.IsServiceUnavailable(err), ensuring
apierrors.IsServiceUnavailable errors are handled by NewRetryRequiredError
instead.
Comment on lines
+19
to
+30
| // 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. | ||
| func HasObjectChanged(desired, fetched client.Object) bool { | ||
| if reflect.TypeOf(desired) != reflect.TypeOf(fetched) { | ||
| return false | ||
| } | ||
| return ObjectMetadataModified(desired, fetched) | ||
| } | ||
|
|
||
| // ObjectMetadataModified compares the labels of two objects and returns true if they differ. | ||
| func ObjectMetadataModified(desired, fetched client.Object) bool { | ||
| return !reflect.DeepEqual(desired.GetLabels(), fetched.GetLabels()) |
There was a problem hiding this comment.
HasObjectChanged currently misses most meaningful drift.
This only delegates to ObjectMetadataModified, and that function only compares labels. Spec, data, annotations, and other metadata changes will all be treated as "no change", which is risky for a helper with this name and docstring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/ctrlutil/utils.go` around lines 19 - 30, HasObjectChanged
currently only checks labels via ObjectMetadataModified; update it to detect
real drift by comparing full metadata (labels, annotations, ownerReferences,
finalizers) and the resource body (spec/status) rather than just labels. Modify
ObjectMetadataModified to include GetAnnotations(), GetOwnerReferences() and
GetFinalizers() in the DeepEqual comparison, and in HasObjectChanged add a
second check that deep-compares the objects' runtime parts: create
metadata-stripped copies of desired and fetched (zero out ObjectMeta fields) and
use reflect.DeepEqual on those copies to detect spec-level changes; return true
if either metadata or stripped-object comparisons differ. Ensure you reference
and change the HasObjectChanged and ObjectMetadataModified functions so both
metadata and spec differences are caught.
Comment on lines
+23
to
+28
| var ( | ||
| errUnsupportedArg = errors.New("validation failed due to unsupported arg") | ||
| errUnsupportedLabel = errors.New("validation failed due to unsupported label") | ||
| errUnsupportedResourceLimits = errors.New("validation failed due to unsupported resource limits") | ||
| errUnsupportedResourceRequests = errors.New("validation failed due to unsupported resource requests") | ||
| ) |
There was a problem hiding this comment.
Use an env-specific sentinel here.
validateOverrideEnv now wraps unsupported env vars with errUnsupportedArg, so env validation failures become indistinguishable from CLI-arg failures and the resulting message is misleading.
Proposed fix
var (
errUnsupportedArg = errors.New("validation failed due to unsupported arg")
+ errUnsupportedEnv = errors.New("validation failed due to unsupported env")
errUnsupportedLabel = errors.New("validation failed due to unsupported label")
errUnsupportedResourceLimits = errors.New("validation failed due to unsupported resource limits")
errUnsupportedResourceRequests = errors.New("validation failed due to unsupported resource requests")
)
func validateOverrideEnv(envMap map[string]corev1.EnvVar, supportedEnv []string) error {
for k, v := range envMap {
if !slices.Contains(supportedEnv, k) {
- return fmt.Errorf("%q=%q: %w", k, v.Value, errUnsupportedArg)
+ return fmt.Errorf("%q=%q: %w", k, v.Value, errUnsupportedEnv)
}
}
return nil
}Also applies to: 122-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/deployment/deployment_overrides_validation.go` around lines 23
- 28, The validateOverrideEnv function is returning errUnsupportedArg for
unsupported environment variables which conflates env validation failures with
CLI-arg failures; add a distinct sentinel error (e.g. errUnsupportedEnv)
alongside the existing errUnsupportedArg/errUnsupportedLabel/etc., have
validateOverrideEnv return errUnsupportedEnv for env-specific unsupported cases,
and update any callers/aggregation logic that currently checks for
errUnsupportedArg (including the code around validateOverrideEnv and the similar
checks at lines referenced ~122-126) to treat errUnsupportedEnv separately so
messages remain accurate.
Comment on lines
+652
to
+660
| // rejectAlreadyMarkedInstance handles an istiocsr that already has the processing-rejected annotation. | ||
| func (r *Reconciler) rejectAlreadyMarkedInstance(istiocsr *v1alpha1.IstioCSR, statusMessage string) error { | ||
| r.log.V(4).Info("%s/%s istiocsr resource contains processing rejected annotation", istiocsr.Namespace, istiocsr.Name) | ||
| var updateErr error | ||
| if istiocsr.Status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, v1alpha1.ReasonFailed, statusMessage) { | ||
| updateErr = r.updateCondition(istiocsr, nil) | ||
| } | ||
| return common.NewMultipleInstanceError(utilerrors.NewAggregate([]error{fmt.Errorf("%s: %w", statusMessage, errMultipleISRInstances), updateErr})) | ||
| } |
There was a problem hiding this comment.
Structured log message contains unsubstituted format specifiers.
Line 654 uses %s/%s in the log message, but structured logging (via logr) doesn't perform printf-style substitution. The placeholders will appear literally in the output.
🔧 Proposed fix
func (r *Reconciler) rejectAlreadyMarkedInstance(istiocsr *v1alpha1.IstioCSR, statusMessage string) error {
- r.log.V(4).Info("%s/%s istiocsr resource contains processing rejected annotation", istiocsr.Namespace, istiocsr.Name)
+ r.log.V(4).Info("istiocsr resource contains processing rejected annotation", "namespace", istiocsr.Namespace, "name", istiocsr.Name)
var updateErr error🤖 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 652 - 660, The log call in
rejectAlreadyMarkedInstance uses printf placeholders ("%s/%s") which won't be
substituted by logr; replace the r.log.V(4).Info call to use structured
key/value pairs instead (e.g., a plain message like "istiocsr resource contains
processing rejected annotation" and keys "namespace", istiocsr.Namespace and
"name", istiocsr.Name) so the Namespace and Name appear correctly in logs;
update the r.log.V(4).Info invocation accordingly in the
rejectAlreadyMarkedInstance function.
Comment on lines
+116
to
+120
| sa, ok := obj.(*corev1.ServiceAccount) | ||
| if !ok { | ||
| panic(fmt.Sprintf("decoded object is not a ServiceAccount: got %T", obj)) | ||
| } | ||
| return sa |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all call sites that must be updated to handle the new error return.
# Expected: every usage of decodeServiceAccountObjBytes is identified for propagation checks.
rg -nP -C3 '\bdecodeServiceAccountObjBytes\s*\('Repository: openshift/cert-manager-operator
Length of output: 3688
🏁 Script executed:
cat -n pkg/controller/trustmanager/utils.go | sed -n '105,125p'Repository: openshift/cert-manager-operator
Length of output: 905
🏁 Script executed:
cat -n pkg/controller/trustmanager/serviceaccounts.go | sed -n '48,58p'Repository: openshift/cert-manager-operator
Length of output: 501
🏁 Script executed:
rg -nP '\bgetServiceAccountObject\s*\(' pkg/controller/trustmanager/Repository: openshift/cert-manager-operator
Length of output: 332
🏁 Script executed:
cat -n pkg/controller/trustmanager/serviceaccounts.go | sed -n '10,25p'Repository: openshift/cert-manager-operator
Length of output: 1046
🏁 Script executed:
cat -n pkg/controller/trustmanager/serviceaccounts.go | sed -n '15,30p'Repository: openshift/cert-manager-operator
Length of output: 1015
🏁 Script executed:
cat -n pkg/controller/trustmanager/serviceaccounts.go | head -1Repository: openshift/cert-manager-operator
Length of output: 102
🏁 Script executed:
rg -n 'serviceAccountAssetName' pkg/controller/trustmanager/Repository: openshift/cert-manager-operator
Length of output: 345
🏁 Script executed:
cat -n pkg/controller/trustmanager/constants.go | sed -n '60,70p'Repository: openshift/cert-manager-operator
Length of output: 323
Change function signature to return error instead of panicking in reconciliation path.
Panicking in decodeServiceAccountObjBytes (lines 114, 118) will crash the operator on malformed or unexpected decoded data. This function is called from createOrApplyServiceAccounts (line 52), which is the reconciliation entry point.
Update the function to return (*corev1.ServiceAccount, error) and propagate errors through the reconciliation flow instead of panicking. The call site at serviceaccounts.go:52 and its caller createOrApplyServiceAccounts must handle the returned error.
Expected changes
-func decodeServiceAccountObjBytes(objBytes []byte) *corev1.ServiceAccount {
+func decodeServiceAccountObjBytes(objBytes []byte) (*corev1.ServiceAccount, error) {
obj, err := runtime.Decode(codecs.UniversalDecoder(corev1.SchemeGroupVersion), objBytes)
if err != nil {
- panic(err)
+ return nil, fmt.Errorf("failed to decode ServiceAccount object bytes: %w", err)
}
sa, ok := obj.(*corev1.ServiceAccount)
if !ok {
- panic(fmt.Sprintf("decoded object is not a ServiceAccount: got %T", obj))
+ return nil, fmt.Errorf("decoded object is not a ServiceAccount: got %T", obj)
}
- return sa
+ return sa, nil
}Update call site in serviceaccounts.go:52:
- serviceAccount := decodeServiceAccountObjBytes(assets.MustAsset(serviceAccountAssetName))
+ serviceAccount, err := decodeServiceAccountObjBytes(assets.MustAsset(serviceAccountAssetName))
+ if err != nil {
+ return err
+ }🤖 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 116 - 120, Change
decodeServiceAccountObjBytes to return (*corev1.ServiceAccount, error) instead
of panicking: replace the panic branch with a descriptive error (e.g.,
fmt.Errorf("decoded object is not a ServiceAccount: got %T", obj)) and return
(nil, err) on failure; on success return (sa, nil). Update the call site in
createOrApplyServiceAccounts (and its caller) to accept the (sa, err) signature,
check and propagate or handle the error in the reconciliation path rather than
letting it panic, and ensure any logging or requeue behavior follows the
operator's error-handling conventions.
da5ac4e to
6b313e8
Compare
openshift-ci
bot
added
the
needs-rebase
|
@anandkuma77: This pull request references CM-902 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. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (12)
pkg/controller/certmanager/deployment_overrides_validation.go (1)
23-28:⚠️ Potential issue | 🟡 MinorAdd a dedicated env sentinel instead of reusing the arg one.
Unsupported env vars are still wrapped with
errUnsupportedArg, so env validation failures remain indistinguishable from CLI-arg failures and the resulting message is misleading.Suggested fix
var ( errUnsupportedArg = errors.New("validation failed due to unsupported arg") + errUnsupportedEnv = errors.New("validation failed due to unsupported env") errUnsupportedLabel = errors.New("validation failed due to unsupported label") errUnsupportedResourceLimits = errors.New("validation failed due to unsupported resource limits") errUnsupportedResourceRequests = errors.New("validation failed due to unsupported resource requests") ) func validateOverrideEnv(envMap map[string]corev1.EnvVar, supportedEnv []string) error { for k, v := range envMap { if !slices.Contains(supportedEnv, k) { - return fmt.Errorf("%q=%q: %w", k, v.Value, errUnsupportedArg) + return fmt.Errorf("%q=%q: %w", k, v.Value, errUnsupportedEnv) } } return nil }Also applies to: 122-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/certmanager/deployment_overrides_validation.go` around lines 23 - 28, Add a dedicated sentinel error for unsupported environment variables instead of reusing errUnsupportedArg: declare var errUnsupportedEnv = errors.New("validation failed due to unsupported env") alongside the other sentinels and replace all places that currently return or wrap errUnsupportedArg for env validation failures (search for env validation routines that reference errUnsupportedArg around the other validation helpers) to return/wrap errUnsupportedEnv instead; ensure any error checks/comparisons that expect the arg sentinel are updated where they should distinguish env vs arg errors so messages become correct.pkg/controller/trustmanager/utils.go (1)
111-120:⚠️ Potential issue | 🟠 MajorReturn an error here instead of panicking.
A decode/type mismatch should surface as a reconcile error, not terminate the operator process. Please change this helper to return
(*corev1.ServiceAccount, error)and propagate that from the caller.Suggested change
-func decodeServiceAccountObjBytes(objBytes []byte) *corev1.ServiceAccount { +func decodeServiceAccountObjBytes(objBytes []byte) (*corev1.ServiceAccount, error) { obj, err := runtime.Decode(codecs.UniversalDecoder(corev1.SchemeGroupVersion), objBytes) if err != nil { - panic(err) + return nil, fmt.Errorf("failed to decode ServiceAccount object bytes: %w", err) } sa, ok := obj.(*corev1.ServiceAccount) if !ok { - panic(fmt.Sprintf("decoded object is not a ServiceAccount: got %T", obj)) + return nil, fmt.Errorf("decoded object is not a ServiceAccount: got %T", obj) } - return sa + 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 111 - 120, The helper decodeServiceAccountObjBytes currently panics on decode/type failures; change its signature to return (*corev1.ServiceAccount, error) and update its internals to return a descriptive error instead of panicking (wrap runtime.Decode errors and return fmt.Errorf for type assertion failures). Update all callers of decodeServiceAccountObjBytes to handle the returned error and propagate it as a reconcile error (or wrap it with context) rather than allowing a panic; ensure callers check the error before using the *corev1.ServiceAccount result.pkg/controller/istiocsr/utils.go (4)
654-655:⚠️ Potential issue | 🟡 MinorUse structured fields with
logr.Info.
logrwon't substitute%s/%shere, so the namespace/name never make it into the log as intended.🔧 Suggested fix
- r.log.V(4).Info("%s/%s istiocsr resource contains processing rejected annotation", istiocsr.Namespace, istiocsr.Name) + r.log.V(4).Info("istiocsr resource contains processing rejected annotation", + "namespace", istiocsr.Namespace, + "name", istiocsr.Name, + )Does logr.Logger.Info support printf-style format strings like "%s/%s", or should structured key/value pairs be used instead?🤖 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 654 - 655, The log call using r.log.V(4).Info("%s/%s istiocsr resource contains processing rejected annotation", istiocsr.Namespace, istiocsr.Name) is incorrect because logr.Info does not perform printf substitution; replace the printf-style call with a structured key/value call such as r.log.V(4).Info("istiocsr resource contains processing rejected annotation", "namespace", istiocsr.Namespace, "name", istiocsr.Name) so the namespace and name fields are included correctly; update the call site in utils.go where r.log.V(4).Info is invoked for the istiocsr object to use structured fields instead of format strings.
433-437:⚠️ Potential issue | 🟠 MajorGuard
Spec.Replicasbefore dereference.
DeploymentSpec.Replicasis optional. If either side is nil, this drift check panics during reconciliation.🔧 Suggested fix
- if *desired.Spec.Replicas != *fetched.Spec.Replicas || - !reflect.DeepEqual(desired.Spec.Selector.MatchLabels, fetched.Spec.Selector.MatchLabels) { + if (desired.Spec.Replicas == nil) != (fetched.Spec.Replicas == nil) { + return true + } + if desired.Spec.Replicas != nil && *desired.Spec.Replicas != *fetched.Spec.Replicas { + return true + } + if !reflect.DeepEqual(desired.Spec.Selector.MatchLabels, fetched.Spec.Selector.MatchLabels) { return true }🤖 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 433 - 437, deploymentSpecModified currently dereferences desired.Spec.Replicas and fetched.Spec.Replicas without nil checks causing a panic when either is nil; update the function to guard both desired.Spec.Replicas and fetched.Spec.Replicas before dereferencing (e.g., treat nil vs non-nil as a difference, and when both non-nil compare values), and keep the existing label selector DeepEqual check; modify the logic in deploymentSpecModified to first check nil-ness of desired.Spec.Replicas and fetched.Spec.Replicas, then compare values only when both are non-nil.
497-506:⚠️ Potential issue | 🟠 MajorNil-safe the probe and security-context comparisons.
ReadinessProbe,ReadinessProbe.HTTPGet, andSecurityContextare optional. These direct dereferences can panic on valid fetched objects.🔧 Suggested fix
func containerReadinessProbeModified(desired, fetched corev1.Container) bool { + if desired.ReadinessProbe == nil || fetched.ReadinessProbe == nil { + return desired.ReadinessProbe != fetched.ReadinessProbe + } + if desired.ReadinessProbe.HTTPGet == nil || fetched.ReadinessProbe.HTTPGet == nil { + return desired.ReadinessProbe.HTTPGet != fetched.ReadinessProbe.HTTPGet + } return desired.ReadinessProbe.HTTPGet.Path != fetched.ReadinessProbe.HTTPGet.Path || desired.ReadinessProbe.InitialDelaySeconds != fetched.ReadinessProbe.InitialDelaySeconds || desired.ReadinessProbe.PeriodSeconds != fetched.ReadinessProbe.PeriodSeconds } func containerResourcesModified(desired, fetched corev1.Container) bool { + if desired.SecurityContext == nil || fetched.SecurityContext == nil { + return desired.SecurityContext != fetched.SecurityContext || + !reflect.DeepEqual(desired.Resources, fetched.Resources) || + !reflect.DeepEqual(desired.VolumeMounts, fetched.VolumeMounts) + } return !reflect.DeepEqual(desired.Resources, fetched.Resources) || !reflect.DeepEqual(*desired.SecurityContext, *fetched.SecurityContext) || !reflect.DeepEqual(desired.VolumeMounts, fetched.VolumeMounts) }🤖 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 497 - 506, The comparisons in containerReadinessProbeModified and containerResourcesModified can panic because ReadinessProbe, ReadinessProbe.HTTPGet and SecurityContext may be nil; update containerReadinessProbeModified to nil-check desired.ReadinessProbe and fetched.ReadinessProbe and their HTTPGet fields before accessing Path/InitialDelaySeconds/PeriodSeconds (treat missing probes as unequal only when the other exists, and compare values only when both non-nil), and update containerResourcesModified to nil-check desired.SecurityContext and fetched.SecurityContext (return unequal if one is nil and the other not, otherwise use reflect.DeepEqual on the dereferenced SecurityContext or simply reflect.DeepEqual on the pointer values) while preserving the existing Resources and VolumeMounts comparisons; refer to the functions containerReadinessProbeModified and containerResourcesModified when making these changes.
637-645:⚠️ Potential issue | 🟠 MajorSkip only the current IstioCSR, not every object in its namespace.
This ignores sibling IstioCSR resources in the same namespace, so multiple instances there can all look “oldest” and bypass rejection.
🔧 Suggested fix
for _, item := range items { - if item.GetNamespace() == istiocsr.Namespace { + if item.GetNamespace() == istiocsr.GetNamespace() && item.GetName() == istiocsr.GetName() { continue }🤖 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 637 - 645, The current loop is skipping every object in the same namespace by comparing item.GetNamespace() == istiocsr.Namespace; change this to skip only the same IstioCSR instance by comparing the object's identity (e.g., item.GetName() and item.GetNamespace(), or item.GetUID()) against istiocsr (the current object) so only the exact same resource is ignored; update the conditional in the loop that now uses item.GetNamespace() and the CreationTimestamp comparison to instead first check identity (same Name+Namespace or UID) and continue only for that exact match, leaving other same-namespace items to be evaluated for oldest determination.pkg/controller/istiocsr/rbacs.go (2)
293-300:⚠️ Potential issue | 🟡 MinorOnly log the unchanged-state message when the RBAC object exists.
This
elsealso runs for the create path, so a brand-new Role/RoleBinding is logged as already existing before it is created.🔧 Suggested fix
if exist && hasObjectChanged(desired, fetched) { r.log.V(1).Info(resourceKind+" has been modified, updating to desired state", "name", resourceName) if err := r.UpdateWithRetry(r.ctx, desired); err != nil { return common.FromClientError(err, "failed to update %s %s", resourceName, resourceKind) } r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "%s %s reconciled back to desired state", resourceKind, resourceName) - } else { + } else if exist { r.log.V(4).Info(resourceKind+" already exists and is in expected state", "name", resourceName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/rbacs.go` around lines 293 - 300, The unchanged-state log is currently in the else branch that also covers the create path; change the logic so r.log.V(4).Info(resourceKind+" already exists and is in expected state", "name", resourceName) is only executed when exist is true and hasObjectChanged(desired, fetched) is false (i.e., the RBAC object exists and did not change). Locate the block using exist, hasObjectChanged(desired, fetched), r.UpdateWithRetry, and r.log.V(4).Info in rbacs.go and either add an explicit if exist { ... } around the unchanged log or convert the if/else into if exist { if hasObjectChanged(...) { update... } else { log unchanged } } to avoid logging "already exists" for new creates.
75-86:⚠️ Potential issue | 🟠 MajorFall back to label-based resolution when the status name is stale.
If
statusNameis set but the referenced cluster-scoped object no longer exists, this returnsexist=falseimmediately and never runslistAndResolve(). That can create a second ClusterRole/ClusterRoleBinding instead of reusing the label-matched one.🔧 Suggested fix
if statusName != "" { key := client.ObjectKey{ Name: statusName, Namespace: desired.GetNamespace(), } exist, err := r.Exists(r.ctx, key, fetched) if err != nil { name := fmt.Sprintf("%s/%s", desired.GetNamespace(), statusName) return false, common.FromClientError(err, "failed to check %s %s resource already exists", name, resourceKind) } - return exist, nil + if exist { + return true, nil + } } return listAndResolve()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/rbacs.go` around lines 75 - 86, The current branch that checks statusName returns exist=false immediately when the referenced cluster-scoped object is missing, which prevents falling back to label-based resolution and can create duplicate ClusterRole/ClusterRoleBinding; change the logic in pkg/controller/istiocsr/rbacs.go so that after calling r.Exists(r.ctx, key, fetched) you treat a NotFound response as a non-fatal case and call listAndResolve() (the same resolution path used when statusName is empty) instead of returning false immediately — use apierrors.IsNotFound(err) (or equivalent client NotFound detection) to decide to fall back, preserve other error handling (returning errors via common.FromClientError), and ensure listAndResolve() is invoked with the same desired, resourceKind and fetched context so label-matched objects are reused.pkg/controller/istiocsr/controller.go (2)
248-251:⚠️ Potential issue | 🟡 MinorDon't use printf placeholders in
logr.Error.The
%sverbs are logged literally here, so the label/value/resource context is lost on invalid watch labels.🔧 Suggested fix
- r.log.Error(errInvalidLabelFormat, "%s label value(%s) not in expected format on %s resource", IstiocsrResourceWatchLabelName, value, obj.GetName()) + r.log.Error(errInvalidLabelFormat, "label value not in expected format", + "label", IstiocsrResourceWatchLabelName, + "value", value, + "resource", obj.GetName(), + )What is the logr.Logger.Error signature, and does it support printf-style message formatting?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/controller.go` around lines 248 - 251, The log call in the invalid-label branch uses printf-style verbs but logr.Logger.Error does not do printf formatting; update the r.log.Error call (referencing r.log.Error, errInvalidLabelFormat, IstiocsrResourceWatchLabelName, value, obj.GetName(), and the key variable) to pass a plain message string and then structured key/value pairs for context (e.g., labels like "label", "value", "resource") instead of "%s" placeholders so the label, value and resource name are correctly recorded by the logger.
88-102:⚠️ Potential issue | 🟠 MajorFinalizer removal still orphans managed resources.
Reconcilenow removes the finalizer as soon ascleanUpreturns, butcleanUpis still a no-op. Deleting the CR will therefore leave behind the RBAC and other resources created byreconcileIstioCSRDeployment.If helpful, I can sketch a cleanup helper that deletes the managed resources before finalizer removal.
Also applies to: 190-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/controller.go` around lines 88 - 102, Reconcile currently calls r.cleanUp(...) and removes the finalizer immediately but cleanUp is a no-op, so implement actual resource deletion in the cleanUp method: locate r.cleanUp and ensure it deletes all resources created by reconcileIstioCSRDeployment (ClusterRole, ClusterRoleBinding, ServiceAccount, Deployment, RBAC objects, etc.), return (true, nil) to requeue if deletions are still in progress and (false, nil) only when all managed resources are confirmed deleted; keep removeFinalizer(ctx, istiocsr, finalizer) called only after cleanUp reports complete, and ensure reconcileIstioCSRDeployment resource identifiers/names are used to find and delete the exact objects to avoid orphaning.pkg/controller/istiocsr/deployments.go (1)
364-382:⚠️ Potential issue | 🟠 MajorDon't reclassify internal CA/issuer validation failures as client errors.
errFailedToConvertToClusterIssuer,errFailedToConvertToIssuer,errIssuerCANotConfigured, anderrFailedToFindCACertificateare internal/configuration failures, not Kubernetes client errors. Wrapping them withcommon.FromClientError(or returning them as plain errors and later doing so) makes permanent misconfiguration look retryable, so reconciliation will keep requeueing instead of surfacing a terminal failure.🔧 Suggested direction
- return certmanagerv1.IssuerConfig{}, common.FromClientError(errFailedToConvertToClusterIssuer, "failed to fetch issuer") + return certmanagerv1.IssuerConfig{}, common.NewIrrecoverableError(errFailedToConvertToClusterIssuer, "failed to fetch issuer") ... - return certmanagerv1.IssuerConfig{}, common.FromClientError(errFailedToConvertToIssuer, "failed to fetch issuer") + return certmanagerv1.IssuerConfig{}, common.NewIrrecoverableError(errFailedToConvertToIssuer, "failed to fetch issuer")and return typed irrecoverable errors from the CA-config validation branches before they reach
handleIssuerBasedCA.Also applies to: 397-406, 533-557
🤖 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 364 - 382, The code is incorrectly wrapping internal/configuration errors (e.g., errFailedToConvertToClusterIssuer, errFailedToConvertToIssuer, errIssuerCANotConfigured, errFailedToFindCACertificate) with common.FromClientError which makes permanent misconfiguration look retryable; update getIssuerConfig and the other CA-validation branches (including the blocks around 397-406 and 533-557) to return a typed irrecoverable/terminal error (or the plain sentinel error) instead of wrapping with common.FromClientError so these errors are treated as terminal by handleIssuerBasedCA and reconciliation; locate the checks in getIssuerConfig, the ClusterIssuer/Issuer cast branches, and the CA certificate validation code and return the internal error types directly (or wrap them in a clearly non-retryable error type) rather than converting them to client errors.pkg/controller/ctrlutil/errors.go (1)
74-76:⚠️ Potential issue | 🟠 MajorTreat
ServiceUnavailableas retryable.
HandleReconcileResultturns irrecoverable errors into a no-requeue path, so classifyingapierrors.IsServiceUnavailable(err)here makes transient 503s fail permanently instead of backing off.🔧 Suggested fix
if apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err) || apierrors.IsInvalid(err) || - apierrors.IsBadRequest(err) || apierrors.IsServiceUnavailable(err) { + apierrors.IsBadRequest(err) { return NewIrrecoverableError(err, message, args...) }In Kubernetes apimachinery, should apierrors.IsServiceUnavailable(err) be treated as retryable in controller reconciliation, or as a permanent/irrecoverable failure?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/ctrlutil/errors.go` around lines 74 - 76, The code incorrectly treats apierrors.IsServiceUnavailable(err) as an irrecoverable error, causing transient 503 responses to be dropped instead of retried; remove apierrors.IsServiceUnavailable(err) from the irrecoverable check (the condition that calls NewIrrecoverableError) so ServiceUnavailable errors are not converted to irrecoverable and will be handled by the normal retry/backoff in HandleReconcileResult; ensure the condition keeps other checks (IsUnauthorized, IsForbidden, IsInvalid, IsBadRequest) intact and that NewIrrecoverableError is only invoked for truly permanent errors.
🧹 Nitpick comments (2)
pkg/operator/utils/apidiscovery_test.go (1)
52-52: Assert the sentinel error in the test path.Now that the fake returns
errExpectedFoo, the negative test should assertrequire.ErrorIs(t, err, errExpectedFoo)(instead of onlyrequire.Error) to catch wrapping/propagation regressions.Also applies to: 59-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/utils/apidiscovery_test.go` at line 52, The test currently only checks for a generic error where the fake returns the sentinel errExpectedFoo; update the negative assertions to assert the specific sentinel by replacing require.Error(...) with require.ErrorIs(t, err, errExpectedFoo) (apply the same change for the other occurrences noted around the test, e.g., the checks at lines 59-60) so the test fails on wrapping/propagation regressions and verifies that errExpectedFoo is preserved.pkg/controller/istiocsr/deployments.go (1)
651-672: Reuse the existing common validation helpers instead of duplicating them.These helpers are effectively copies of
pkg/controller/common/validation.go, so the validation logic is now split across two places with the sameunsafeconversions and Kubernetes validators. Reusing the common helpers keeps one source of truth.🤖 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 651 - 672, Replace the duplicated validation implementations in validateNodeSelectorConfig, validateTolerationsConfig, validateResourceRequirements, and validateAffinityRules with calls to the shared helpers in pkg/controller/common/validation.go; locate the equivalent common helper functions (the nodeSelector, tolerations, resource requirements and affinity validators) and call them from these wrappers (passing the same parameters and fldPath.Child(...) as before) instead of redoing unsafe conversions and validation logic locally so there is a single source of truth.
🤖 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/certmanager/credentials_request.go`:
- Around line 128-133: The code sets platformType from
infra.Status.PlatformStatus.Type and calls
cloudCredentialsForPlatform(platformType, secretName) but doesn't fall back to
the deprecated infra.Status.Platform when PlatformStatus is nil; update the
logic where platformType is assigned so that if infra.Status.PlatformStatus is
nil you assign platformType = infra.Status.Platform (the deprecated string/enum)
before calling cloudCredentialsForPlatform, ensuring cloudCredentialsForPlatform
receives a valid platformType and avoids the unsupported-provider default case.
- Around line 142-150: The code directly indexes
deployment.Spec.Template.Spec.Containers[0] when appending volume mounts and env
vars which can panic if the containers slice is empty; before those append
operations in credentials_request.go, check
len(deployment.Spec.Template.Spec.Containers) > 0 and return a descriptive error
(e.g., "deployment has no containers in pod template") if not, then proceed to
append config.volumeMount and *config.envVar to
deployment.Spec.Template.Spec.Containers[0]; apply the same guard for both the
volumeMount and env var append sites to avoid runtime panics.
In `@pkg/controller/istiocsr/utils.go`:
- Around line 250-263: The log "resource already exists and is in expected
state" is currently in an else that also runs when exist == false; change the
conditional so that the "already exists" message is only emitted when exist is
true and hasObjectChanged(desired, fetched) is false (e.g., use if exist { if
hasObjectChanged(...) { ... } else { r.log.V(4).Info(...) } } else { create...
}), keeping the update path using r.UpdateWithRetry and the create path using
r.Create and preserving eventRecorder calls.
In `@pkg/operator/starter.go`:
- Around line 37-39: The package-level exported variable and its doc comment are
inconsistent: the comment refers to CloudSecretName while the variable is named
CloudCredentialSecret; pick one canonical name (prefer CloudSecretName to match
the comment) and rename the variable (CloudCredentialSecret -> CloudSecretName)
in pkg/operator/starter.go and update all usages across the repo
(imports/usages, tests, and any runtime args) to the new identifier, or
alternatively update the comment to match CloudCredentialSecret—ensure the doc
comment and the exported variable name are identical and adjust references
accordingly.
- Around line 165-185: startOperandControllers currently fires
manager.Start(ctx) in a goroutine and only logs failures, so runtime errors are
lost; change its API to return a manager error channel (e.g., make it return
(chan error, error) or (<-chan error, error)), create a managerErrCh, run the
goroutine to call manager.Start(ctx) and send any non-nil error into
managerErrCh (instead of only logging), and return managerErrCh to the caller so
RunOperator can select on that channel and propagate/handle runtime manager
failures; reference startOperandControllers and manager.Start when making this
change.
---
Duplicate comments:
In `@pkg/controller/certmanager/deployment_overrides_validation.go`:
- Around line 23-28: Add a dedicated sentinel error for unsupported environment
variables instead of reusing errUnsupportedArg: declare var errUnsupportedEnv =
errors.New("validation failed due to unsupported env") alongside the other
sentinels and replace all places that currently return or wrap errUnsupportedArg
for env validation failures (search for env validation routines that reference
errUnsupportedArg around the other validation helpers) to return/wrap
errUnsupportedEnv instead; ensure any error checks/comparisons that expect the
arg sentinel are updated where they should distinguish env vs arg errors so
messages become correct.
In `@pkg/controller/ctrlutil/errors.go`:
- Around line 74-76: The code incorrectly treats
apierrors.IsServiceUnavailable(err) as an irrecoverable error, causing transient
503 responses to be dropped instead of retried; remove
apierrors.IsServiceUnavailable(err) from the irrecoverable check (the condition
that calls NewIrrecoverableError) so ServiceUnavailable errors are not converted
to irrecoverable and will be handled by the normal retry/backoff in
HandleReconcileResult; ensure the condition keeps other checks (IsUnauthorized,
IsForbidden, IsInvalid, IsBadRequest) intact and that NewIrrecoverableError is
only invoked for truly permanent errors.
In `@pkg/controller/istiocsr/controller.go`:
- Around line 248-251: The log call in the invalid-label branch uses
printf-style verbs but logr.Logger.Error does not do printf formatting; update
the r.log.Error call (referencing r.log.Error, errInvalidLabelFormat,
IstiocsrResourceWatchLabelName, value, obj.GetName(), and the key variable) to
pass a plain message string and then structured key/value pairs for context
(e.g., labels like "label", "value", "resource") instead of "%s" placeholders so
the label, value and resource name are correctly recorded by the logger.
- Around line 88-102: Reconcile currently calls r.cleanUp(...) and removes the
finalizer immediately but cleanUp is a no-op, so implement actual resource
deletion in the cleanUp method: locate r.cleanUp and ensure it deletes all
resources created by reconcileIstioCSRDeployment (ClusterRole,
ClusterRoleBinding, ServiceAccount, Deployment, RBAC objects, etc.), return
(true, nil) to requeue if deletions are still in progress and (false, nil) only
when all managed resources are confirmed deleted; keep removeFinalizer(ctx,
istiocsr, finalizer) called only after cleanUp reports complete, and ensure
reconcileIstioCSRDeployment resource identifiers/names are used to find and
delete the exact objects to avoid orphaning.
In `@pkg/controller/istiocsr/deployments.go`:
- Around line 364-382: The code is incorrectly wrapping internal/configuration
errors (e.g., errFailedToConvertToClusterIssuer, errFailedToConvertToIssuer,
errIssuerCANotConfigured, errFailedToFindCACertificate) with
common.FromClientError which makes permanent misconfiguration look retryable;
update getIssuerConfig and the other CA-validation branches (including the
blocks around 397-406 and 533-557) to return a typed irrecoverable/terminal
error (or the plain sentinel error) instead of wrapping with
common.FromClientError so these errors are treated as terminal by
handleIssuerBasedCA and reconciliation; locate the checks in getIssuerConfig,
the ClusterIssuer/Issuer cast branches, and the CA certificate validation code
and return the internal error types directly (or wrap them in a clearly
non-retryable error type) rather than converting them to client errors.
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 293-300: The unchanged-state log is currently in the else branch
that also covers the create path; change the logic so
r.log.V(4).Info(resourceKind+" already exists and is in expected state", "name",
resourceName) is only executed when exist is true and hasObjectChanged(desired,
fetched) is false (i.e., the RBAC object exists and did not change). Locate the
block using exist, hasObjectChanged(desired, fetched), r.UpdateWithRetry, and
r.log.V(4).Info in rbacs.go and either add an explicit if exist { ... } around
the unchanged log or convert the if/else into if exist { if
hasObjectChanged(...) { update... } else { log unchanged } } to avoid logging
"already exists" for new creates.
- Around line 75-86: The current branch that checks statusName returns
exist=false immediately when the referenced cluster-scoped object is missing,
which prevents falling back to label-based resolution and can create duplicate
ClusterRole/ClusterRoleBinding; change the logic in
pkg/controller/istiocsr/rbacs.go so that after calling r.Exists(r.ctx, key,
fetched) you treat a NotFound response as a non-fatal case and call
listAndResolve() (the same resolution path used when statusName is empty)
instead of returning false immediately — use apierrors.IsNotFound(err) (or
equivalent client NotFound detection) to decide to fall back, preserve other
error handling (returning errors via common.FromClientError), and ensure
listAndResolve() is invoked with the same desired, resourceKind and fetched
context so label-matched objects are reused.
In `@pkg/controller/istiocsr/utils.go`:
- Around line 654-655: The log call using r.log.V(4).Info("%s/%s istiocsr
resource contains processing rejected annotation", istiocsr.Namespace,
istiocsr.Name) is incorrect because logr.Info does not perform printf
substitution; replace the printf-style call with a structured key/value call
such as r.log.V(4).Info("istiocsr resource contains processing rejected
annotation", "namespace", istiocsr.Namespace, "name", istiocsr.Name) so the
namespace and name fields are included correctly; update the call site in
utils.go where r.log.V(4).Info is invoked for the istiocsr object to use
structured fields instead of format strings.
- Around line 433-437: deploymentSpecModified currently dereferences
desired.Spec.Replicas and fetched.Spec.Replicas without nil checks causing a
panic when either is nil; update the function to guard both
desired.Spec.Replicas and fetched.Spec.Replicas before dereferencing (e.g.,
treat nil vs non-nil as a difference, and when both non-nil compare values), and
keep the existing label selector DeepEqual check; modify the logic in
deploymentSpecModified to first check nil-ness of desired.Spec.Replicas and
fetched.Spec.Replicas, then compare values only when both are non-nil.
- Around line 497-506: The comparisons in containerReadinessProbeModified and
containerResourcesModified can panic because ReadinessProbe,
ReadinessProbe.HTTPGet and SecurityContext may be nil; update
containerReadinessProbeModified to nil-check desired.ReadinessProbe and
fetched.ReadinessProbe and their HTTPGet fields before accessing
Path/InitialDelaySeconds/PeriodSeconds (treat missing probes as unequal only
when the other exists, and compare values only when both non-nil), and update
containerResourcesModified to nil-check desired.SecurityContext and
fetched.SecurityContext (return unequal if one is nil and the other not,
otherwise use reflect.DeepEqual on the dereferenced SecurityContext or simply
reflect.DeepEqual on the pointer values) while preserving the existing Resources
and VolumeMounts comparisons; refer to the functions
containerReadinessProbeModified and containerResourcesModified when making these
changes.
- Around line 637-645: The current loop is skipping every object in the same
namespace by comparing item.GetNamespace() == istiocsr.Namespace; change this to
skip only the same IstioCSR instance by comparing the object's identity (e.g.,
item.GetName() and item.GetNamespace(), or item.GetUID()) against istiocsr (the
current object) so only the exact same resource is ignored; update the
conditional in the loop that now uses item.GetNamespace() and the
CreationTimestamp comparison to instead first check identity (same
Name+Namespace or UID) and continue only for that exact match, leaving other
same-namespace items to be evaluated for oldest determination.
In `@pkg/controller/trustmanager/utils.go`:
- Around line 111-120: The helper decodeServiceAccountObjBytes currently panics
on decode/type failures; change its signature to return (*corev1.ServiceAccount,
error) and update its internals to return a descriptive error instead of
panicking (wrap runtime.Decode errors and return fmt.Errorf for type assertion
failures). Update all callers of decodeServiceAccountObjBytes to handle the
returned error and propagate it as a reconcile error (or wrap it with context)
rather than allowing a panic; ensure callers check the error before using the
*corev1.ServiceAccount result.
---
Nitpick comments:
In `@pkg/controller/istiocsr/deployments.go`:
- Around line 651-672: Replace the duplicated validation implementations in
validateNodeSelectorConfig, validateTolerationsConfig,
validateResourceRequirements, and validateAffinityRules with calls to the shared
helpers in pkg/controller/common/validation.go; locate the equivalent common
helper functions (the nodeSelector, tolerations, resource requirements and
affinity validators) and call them from these wrappers (passing the same
parameters and fldPath.Child(...) as before) instead of redoing unsafe
conversions and validation logic locally so there is a single source of truth.
In `@pkg/operator/utils/apidiscovery_test.go`:
- Line 52: The test currently only checks for a generic error where the fake
returns the sentinel errExpectedFoo; update the negative assertions to assert
the specific sentinel by replacing require.Error(...) with require.ErrorIs(t,
err, errExpectedFoo) (apply the same change for the other occurrences noted
around the test, e.g., the checks at lines 59-60) so the test fails on
wrapping/propagation regressions and verifies that errExpectedFoo is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fc4b02a5-c7c6-4a9d-92df-fcf5d2fac918
📒 Files selected for processing (42)
.golangci.yamlmain.gopkg/controller/certmanager/cert_manager_networkpolicy.gopkg/controller/certmanager/certmanager_controller.gopkg/controller/certmanager/credentials_request.gopkg/controller/certmanager/deployment_helper.gopkg/controller/certmanager/deployment_overrides.gopkg/controller/certmanager/deployment_overrides_validation.gopkg/controller/certmanager/generic_deployment_controller.gopkg/controller/common/client.gopkg/controller/common/constants.gopkg/controller/common/errors.gopkg/controller/common/reconcile_result.gopkg/controller/common/utils.gopkg/controller/ctrlutil/client.gopkg/controller/ctrlutil/constants.gopkg/controller/ctrlutil/errors.gopkg/controller/ctrlutil/fakes/fake_ctrl_client.gopkg/controller/ctrlutil/reconcile_result.gopkg/controller/ctrlutil/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_test.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/utils.gopkg/operator/operatorclient/operatorclient.gopkg/operator/setup_manager.gopkg/operator/starter.gopkg/operator/utils/apidiscovery_test.go
✅ Files skipped from review due to trivial changes (10)
- pkg/controller/common/reconcile_result.go
- pkg/controller/common/constants.go
- pkg/controller/common/utils.go
- main.go
- pkg/controller/ctrlutil/constants.go
- pkg/controller/certmanager/deployment_overrides.go
- pkg/controller/istiocsr/serviceaccounts_test.go
- pkg/controller/istiocsr/controller_test.go
- pkg/controller/istiocsr/certificates_test.go
- pkg/controller/istiocsr/services_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/controller/common/errors.go
- pkg/controller/common/client.go
- pkg/controller/istiocsr/networkpolicies.go
- pkg/operator/setup_manager.go
- pkg/controller/istiocsr/certificates.go
- .golangci.yaml
- pkg/controller/ctrlutil/reconcile_result.go
- pkg/controller/ctrlutil/utils.go
- pkg/controller/trustmanager/controller.go
- pkg/controller/trustmanager/install_trustmanager.go
- pkg/controller/trustmanager/constants.go
- pkg/controller/istiocsr/deployments_test.go
Comment on lines
+128
to
+133
| var platformType configv1.PlatformType | ||
| if infra.Status.PlatformStatus != nil { | ||
| platformType = infra.Status.PlatformStatus.Type | ||
| } | ||
|
|
||
| config, err := cloudCredentialsForPlatform(platformType, secretName) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Infrastructure API field optionality + deprecation guidance.
rg -n -C2 'PlatformStatus \*PlatformStatus|Platform PlatformType|Deprecated: Use platformStatus.type' \
vendor/github.com/openshift/api/config/v1/types_infrastructure.go
# Verify current platform resolution path in this file.
rg -n -C3 'infra.Status.PlatformStatus|platformType = infra.Status.PlatformStatus.Type|cloudCredentialsForPlatform' \
pkg/controller/certmanager/credentials_request.goRepository: openshift/cert-manager-operator
Length of output: 1134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the complete cloudCredentialsForPlatform function to see switch cases
rg -n -A 40 'func cloudCredentialsForPlatform' \
pkg/controller/certmanager/credentials_request.go
# Check if there are any other references to infra.Status.Platform
# to see if fallback pattern is used elsewhere
rg -n 'infra.Status.Platform' \
pkg/controller/certmanager/credentials_request.go
# Look for tests that might cover this scenario
fd -e '.*test.*\.go$' pkg/controller/certmanager/ 2>/dev/null | \
xargs rg -l 'PlatformStatus|Platform' 2>/dev/null || trueRepository: openshift/cert-manager-operator
Length of output: 10652
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get more lines from cloudCredentialsForPlatform to see the full switch/default
rg -n -A 80 'func cloudCredentialsForPlatform' \
pkg/controller/certmanager/credentials_request.go | head -100
# Check the test file for any tests related to this scenario
cat -n pkg/controller/certmanager/credentials_request_test.go | head -200Repository: openshift/cert-manager-operator
Length of output: 11169
Add fallback to deprecated infra.Status.Platform when PlatformStatus is nil.
When PlatformStatus is nil (which is valid since the field is optional), the code leaves platformType empty, triggering the unsupported-provider error at the default case of the cloudCredentialsForPlatform switch statement. Clusters relying on the older infra.Status.Platform field will fail unnecessarily.
Proposed fix
- var platformType configv1.PlatformType
- if infra.Status.PlatformStatus != nil {
- platformType = infra.Status.PlatformStatus.Type
- }
+ platformType := infra.Status.Platform
+ if infra.Status.PlatformStatus != nil {
+ platformType = infra.Status.PlatformStatus.Type
+ }📝 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
| var platformType configv1.PlatformType | |
| if infra.Status.PlatformStatus != nil { | |
| platformType = infra.Status.PlatformStatus.Type | |
| } | |
| config, err := cloudCredentialsForPlatform(platformType, secretName) | |
| platformType := infra.Status.Platform | |
| if infra.Status.PlatformStatus != nil { | |
| platformType = infra.Status.PlatformStatus.Type | |
| } | |
| config, err := cloudCredentialsForPlatform(platformType, secretName) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/certmanager/credentials_request.go` around lines 128 - 133,
The code sets platformType from infra.Status.PlatformStatus.Type and calls
cloudCredentialsForPlatform(platformType, secretName) but doesn't fall back to
the deprecated infra.Status.Platform when PlatformStatus is nil; update the
logic where platformType is assigned so that if infra.Status.PlatformStatus is
nil you assign platformType = infra.Status.Platform (the deprecated string/enum)
before calling cloudCredentialsForPlatform, ensuring cloudCredentialsForPlatform
receives a valid platformType and avoids the unsupported-provider default case.
Comment on lines
+142
to
+150
| deployment.Spec.Template.Spec.Containers[0].VolumeMounts = append( | ||
| deployment.Spec.Template.Spec.Containers[0].VolumeMounts, | ||
| config.volumeMount, | ||
| ) | ||
|
|
||
| if config.envVar != nil { | ||
| deployment.Spec.Template.Spec.Containers[0].Env = append( | ||
| deployment.Spec.Template.Spec.Containers[0].Env, | ||
| *config.envVar, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pkg/controller/certmanager/credentials_request.go | head -200Repository: openshift/cert-manager-operator
Length of output: 5788
🏁 Script executed:
rg -n "applyCloudCredentials" pkg/controller/certmanager/Repository: openshift/cert-manager-operator
Length of output: 458
🏁 Script executed:
rg -n "withCloudCredentials" pkg/controller/certmanager/Repository: openshift/cert-manager-operator
Length of output: 731
🏁 Script executed:
cat -n pkg/controller/certmanager/generic_deployment_controller.go | head -100Repository: openshift/cert-manager-operator
Length of output: 4674
🏁 Script executed:
find pkg/operator/assets -name "*.yaml" -o -name "*.json" | head -20Repository: openshift/cert-manager-operator
Length of output: 57
🏁 Script executed:
rg "MustAsset|assets" pkg/operator/ --type-list | head -20Repository: openshift/cert-manager-operator
Length of output: 464
🏁 Script executed:
fd -t d "assets" pkg/operator/Repository: openshift/cert-manager-operator
Length of output: 95
🏁 Script executed:
ls -la pkg/operator/assets/Repository: openshift/cert-manager-operator
Length of output: 253
🏁 Script executed:
rg "Containers\[0\]" pkg/controller/certmanager/ -A 2 -B 2Repository: openshift/cert-manager-operator
Length of output: 10018
🏁 Script executed:
grep -A 20 "func newDeployment\|var deployment\|func getDeployment" pkg/controller/certmanager/credentials_request_test.go | head -80Repository: openshift/cert-manager-operator
Length of output: 57
🏁 Script executed:
cat -n pkg/controller/certmanager/credentials_request_test.go | head -100Repository: openshift/cert-manager-operator
Length of output: 4261
🏁 Script executed:
cat -n pkg/controller/certmanager/credentials_request_test.go | sed -n '100,180p'Repository: openshift/cert-manager-operator
Length of output: 3489
🏁 Script executed:
cat -n pkg/controller/certmanager/credentials_request_test.go | sed -n '178,220p'Repository: openshift/cert-manager-operator
Length of output: 1872
🏁 Script executed:
cat -n pkg/controller/certmanager/deployment_overrides.go | sed -n '80,140p'Repository: openshift/cert-manager-operator
Length of output: 3465
Guard Containers[0] access to avoid panic on invalid deployment templates.
At lines 142 and 148, indexing deployment.Spec.Template.Spec.Containers[0] can panic if the container list is empty. Returning an error here is safer than crashing reconciliation. This pattern is used defensively elsewhere in the codebase (e.g., deployment_overrides.go checks len(deployment.Spec.Template.Spec.Containers) == 1).
💡 Proposed fix
+ if len(deployment.Spec.Template.Spec.Containers) == 0 {
+ return fmt.Errorf("controller deployment %q has no containers", deployment.Name)
+ }
+ container := &deployment.Spec.Template.Spec.Containers[0]
+
deployment.Spec.Template.Spec.Volumes = append(
deployment.Spec.Template.Spec.Volumes,
config.volume,
)
- deployment.Spec.Template.Spec.Containers[0].VolumeMounts = append(
- deployment.Spec.Template.Spec.Containers[0].VolumeMounts,
+ container.VolumeMounts = append(
+ container.VolumeMounts,
config.volumeMount,
)
if config.envVar != nil {
- deployment.Spec.Template.Spec.Containers[0].Env = append(
- deployment.Spec.Template.Spec.Containers[0].Env,
+ container.Env = append(
+ container.Env,
*config.envVar,
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/certmanager/credentials_request.go` around lines 142 - 150,
The code directly indexes deployment.Spec.Template.Spec.Containers[0] when
appending volume mounts and env vars which can panic if the containers slice is
empty; before those append operations in credentials_request.go, check
len(deployment.Spec.Template.Spec.Containers) > 0 and return a descriptive error
(e.g., "deployment has no containers in pod template") if not, then proceed to
append config.volumeMount and *config.envVar to
deployment.Spec.Template.Spec.Containers[0]; apply the same guard for both the
volumeMount and env var append sites to avoid runtime panics.
Comment on lines
+250
to
+263
| if exist && hasObjectChanged(desired, fetched) { | ||
| r.log.V(1).Info(resourceKind+" has been modified, updating to desired state", "name", resourceName) | ||
| if err := r.UpdateWithRetry(r.ctx, desired); err != nil { | ||
| return common.FromClientError(err, "failed to update %s %s resource", resourceName, resourceKind) | ||
| } | ||
| r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "%s resource %s reconciled back to desired state", resourceKind, resourceName) | ||
| } else { | ||
| r.log.V(4).Info(resourceKind+" resource already exists and is in expected state", "name", resourceName) | ||
| } | ||
| if !exist { | ||
| if err := r.Create(r.ctx, desired); err != nil { | ||
| return common.FromClientError(err, "failed to create %s %s resource", resourceName, resourceKind) | ||
| } | ||
| r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "%s resource %s created", resourceKind, resourceName) |
There was a problem hiding this comment.
Only log “already exists” on the existing-resource path.
This else also runs when exist == false, so a first-time reconcile logs the object as already present right before creating it.
🔧 Suggested fix
if exist && hasObjectChanged(desired, fetched) {
r.log.V(1).Info(resourceKind+" has been modified, updating to desired state", "name", resourceName)
if err := r.UpdateWithRetry(r.ctx, desired); err != nil {
return common.FromClientError(err, "failed to update %s %s resource", resourceName, resourceKind)
}
r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "%s resource %s reconciled back to desired state", resourceKind, resourceName)
- } else {
+ } else if exist {
r.log.V(4).Info(resourceKind+" resource already exists and is in expected state", "name", resourceName)
}📝 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 exist && hasObjectChanged(desired, fetched) { | |
| r.log.V(1).Info(resourceKind+" has been modified, updating to desired state", "name", resourceName) | |
| if err := r.UpdateWithRetry(r.ctx, desired); err != nil { | |
| return common.FromClientError(err, "failed to update %s %s resource", resourceName, resourceKind) | |
| } | |
| r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "%s resource %s reconciled back to desired state", resourceKind, resourceName) | |
| } else { | |
| r.log.V(4).Info(resourceKind+" resource already exists and is in expected state", "name", resourceName) | |
| } | |
| if !exist { | |
| if err := r.Create(r.ctx, desired); err != nil { | |
| return common.FromClientError(err, "failed to create %s %s resource", resourceName, resourceKind) | |
| } | |
| r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "%s resource %s created", resourceKind, resourceName) | |
| if exist && hasObjectChanged(desired, fetched) { | |
| r.log.V(1).Info(resourceKind+" has been modified, updating to desired state", "name", resourceName) | |
| if err := r.UpdateWithRetry(r.ctx, desired); err != nil { | |
| return common.FromClientError(err, "failed to update %s %s resource", resourceName, resourceKind) | |
| } | |
| r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "%s resource %s reconciled back to desired state", resourceKind, resourceName) | |
| } else if exist { | |
| r.log.V(4).Info(resourceKind+" resource already exists and is in expected state", "name", resourceName) | |
| } | |
| if !exist { | |
| if err := r.Create(r.ctx, desired); err != nil { | |
| return common.FromClientError(err, "failed to create %s %s resource", resourceName, resourceKind) | |
| } | |
| r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "%s resource %s created", resourceKind, resourceName) | |
| } |
🤖 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 250 - 263, The log "resource
already exists and is in expected state" is currently in an else that also runs
when exist == false; change the conditional so that the "already exists" message
is only emitted when exist is true and hasObjectChanged(desired, fetched) is
false (e.g., use if exist { if hasObjectChanged(...) { ... } else {
r.log.V(4).Info(...) } } else { create... }), keeping the update path using
r.UpdateWithRetry and the create path using r.Create and preserving
eventRecorder calls.
Comment on lines
37
to
39
| // CloudSecretName is the name of the cloud secret to be | ||
| // used in ambient credentials mode, and is provided as a runtime arg. | ||
| var CloudCredentialSecret string |
There was a problem hiding this comment.
Comment and variable name are inconsistent.
The comment says CloudSecretName, but the variable is still CloudCredentialSecret. Please align them to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/starter.go` around lines 37 - 39, The package-level exported
variable and its doc comment are inconsistent: the comment refers to
CloudSecretName while the variable is named CloudCredentialSecret; pick one
canonical name (prefer CloudSecretName to match the comment) and rename the
variable (CloudCredentialSecret -> CloudSecretName) in pkg/operator/starter.go
and update all usages across the repo (imports/usages, tests, and any runtime
args) to the new identifier, or alternatively update the comment to match
CloudCredentialSecret—ensure the doc comment and the exported variable name are
identical and adjust references accordingly.
Comment on lines
165
to
185
| func startOperandControllers(ctx context.Context, istioCSREnabled, trustManagerEnabled bool) error { | ||
| if !istioCSREnabled && !trustManagerEnabled { | ||
| return nil | ||
| } | ||
|
|
||
| if err := features.SetupWithFlagValue(UnsupportedAddonFeatures); err != nil { | ||
| return nil, fmt.Errorf("failed to parse addon features: %w", err) | ||
| manager, err := NewControllerManager(ControllerConfig{ | ||
| EnableIstioCSR: istioCSREnabled, | ||
| EnableTrustManager: trustManagerEnabled, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create unified controller manager: %w", err) | ||
| } | ||
|
|
||
| for attempt := 1; attempt <= maxFeatureGateAttempts; attempt++ { | ||
| featureStatus = features.NewFeatureGateState(ctx, configClient) | ||
| err := featureStatus.Err() | ||
| if err == nil { | ||
| break | ||
| } | ||
| if attempt == maxFeatureGateAttempts { | ||
| ctrl.Log.Info("feature gate validation failed after max attempts; continuing with last state", | ||
| "error", err, "attempts", attempt) | ||
| break | ||
| go func() { | ||
| if err := manager.Start(ctx); err != nil { | ||
| ctrl.Log.Error(err, "failed to start unified controller manager") | ||
| } | ||
| ctrl.Log.Info("failed to validate featuregate settings, retrying", | ||
| "error", err, "attempt", attempt, "maxAttempts", maxFeatureGateAttempts) | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, fmt.Errorf("context cancelled: %w", ctx.Err()) | ||
| case <-time.After(featureGateRetryDelay): | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| return featureStatus, nil | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Operand manager startup failures are not propagated.
If manager.Start(ctx) fails, the error is only logged and RunOperator keeps running, which can mask a broken operand control-plane.
🔧 Suggested approach to propagate runtime manager failures
-func startOperandControllers(ctx context.Context, istioCSREnabled, trustManagerEnabled bool) error {
+func startOperandControllers(ctx context.Context, istioCSREnabled, trustManagerEnabled bool) (<-chan error, error) {
if !istioCSREnabled && !trustManagerEnabled {
- return nil
+ return nil, nil
}
manager, err := NewControllerManager(ControllerConfig{
EnableIstioCSR: istioCSREnabled,
EnableTrustManager: trustManagerEnabled,
})
if err != nil {
- return fmt.Errorf("failed to create unified controller manager: %w", err)
+ return nil, fmt.Errorf("failed to create unified controller manager: %w", err)
}
+ errCh := make(chan error, 1)
go func() {
- if err := manager.Start(ctx); err != nil {
- ctrl.Log.Error(err, "failed to start unified controller manager")
- }
+ errCh <- manager.Start(ctx)
}()
- return nil
+ return errCh, nil
}- if err := startOperandControllers(ctx, istioCSREnabled, trustManagerEnabled); err != nil {
+ operandErrCh, err := startOperandControllers(ctx, istioCSREnabled, trustManagerEnabled)
+ if err != nil {
return err
}
-
- <-ctx.Done()
- return nil
+ select {
+ case <-ctx.Done():
+ return nil
+ case err := <-operandErrCh:
+ if err != nil {
+ return fmt.Errorf("unified controller manager exited: %w", err)
+ }
+ return nil
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/starter.go` around lines 165 - 185, startOperandControllers
currently fires manager.Start(ctx) in a goroutine and only logs failures, so
runtime errors are lost; change its API to return a manager error channel (e.g.,
make it return (chan error, error) or (<-chan error, error)), create a
managerErrCh, run the goroutine to call manager.Start(ctx) and send any non-nil
error into managerErrCh (instead of only logging), and return managerErrCh to
the caller so RunOperator can select on that channel and propagate/handle
runtime manager failures; reference startOperandControllers and manager.Start
when making this change.
6b313e8 to
1843ff2
Compare
openshift-ci
bot
removed
the
needs-rebase
|
@anandkuma77: This pull request references CM-902 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controller/istiocsr/rbacs.go (1)
316-348: Consider using this helper for the remaining namespaced create/apply paths too.
pkg/controller/istiocsr/serviceaccounts.go:14-33andpkg/controller/istiocsr/services.go:35-55still carry the same exists/update/create flow. Folding them into this helper would finish the dedup and keep reconcile behavior aligned in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/rbacs.go` around lines 316 - 348, The create-or-update logic for namespaced RBAC is already centralized in reconcileNamespacedRBACObject; refactor the duplicated exists/update/create flows in pkg/controller/istiocsr/serviceaccounts.go (lines ~14-33) and pkg/controller/istiocsr/services.go (lines ~35-55) to call that helper instead. Replace the inline logic with calls to r.reconcileNamespacedRBACObject(istiocsr, desiredObj, fetchedEmptyObj, logMsg, resourceKind, istioCSRCreateRecon) ensuring you pass an empty instance of the same type for fetched, appropriate logMsg and resourceKind strings, and preserve the istioCSRCreateRecon boolean; remove the duplicated existence/update/create code paths so reconciliation behavior is unified via the helper.pkg/controller/trustmanager/utils.go (1)
250-267: Differentiate create vs update in the shared SSA logs/events.On the
!existspath, Line 261 still says the resource was “modified” and is being “updated”. Splitting the message by create vs update would make first-reconcile behavior much clearer in operator logs and events.🤖 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 250 - 267, The log/event should distinguish create vs update: after checking exists and before calling r.Patch (the current r.log.V(2).Info and r.eventRecorder.Eventf uses "modified/updated"), branch on the exists boolean and emit different messages and event text — e.g., if !exists log "creating <resourceKind> resource" and send an event like "%s resource %s created", otherwise log "updating <resourceKind> resource" and send an event like "%s resource %s updated"; keep the Patch call (r.Patch(..., client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership)) the same and reuse resourceKind, resourceName, trustManager variables for the messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 316-348: The create-or-update logic for namespaced RBAC is already
centralized in reconcileNamespacedRBACObject; refactor the duplicated
exists/update/create flows in pkg/controller/istiocsr/serviceaccounts.go (lines
~14-33) and pkg/controller/istiocsr/services.go (lines ~35-55) to call that
helper instead. Replace the inline logic with calls to
r.reconcileNamespacedRBACObject(istiocsr, desiredObj, fetchedEmptyObj, logMsg,
resourceKind, istioCSRCreateRecon) ensuring you pass an empty instance of the
same type for fetched, appropriate logMsg and resourceKind strings, and preserve
the istioCSRCreateRecon boolean; remove the duplicated existence/update/create
code paths so reconciliation behavior is unified via the helper.
In `@pkg/controller/trustmanager/utils.go`:
- Around line 250-267: The log/event should distinguish create vs update: after
checking exists and before calling r.Patch (the current r.log.V(2).Info and
r.eventRecorder.Eventf uses "modified/updated"), branch on the exists boolean
and emit different messages and event text — e.g., if !exists log "creating
<resourceKind> resource" and send an event like "%s resource %s created",
otherwise log "updating <resourceKind> resource" and send an event like "%s
resource %s updated"; keep the Patch call (r.Patch(..., client.Apply,
client.FieldOwner(fieldOwner), client.ForceOwnership)) the same and reuse
resourceKind, resourceName, trustManager variables for the messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 036f445d-380f-4e79-8026-1588b157cc08
📒 Files selected for processing (6)
.golangci.yamlpkg/controller/istiocsr/rbacs.gopkg/controller/trustmanager/certificates.gopkg/controller/trustmanager/rbacs.gopkg/controller/trustmanager/utils.gopkg/controller/trustmanager/webhooks.go
openshift-ci
bot
added
the
needs-rebase
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. |
anandkuma77
force-pushed
the
cm-902-golangci-lint-secondpart
branch
from
c8f8ee4 to
16f1a42
Compare
This commit enables additional linters in golangci-lint configuration and fixes all resulting issues: Enabled linters: - dupl: detect duplicate code - err113: enforce static errors - revive: code style and naming conventions - cyclop: cyclomatic complexity - gocognit: cognitive complexity - maintidx: maintainability index Fixes applied: - Fix duplicate code by extracting common helper functions - Add nolint directives for err113 issues with justifications - Fix revive naming issues (remove stuttering, package names) - Fix all revive unused-parameter warnings - Fix misc lint issues: godot, intrange, staticcheck, unparam - Define constants for magic strings Lint status reduced from 92 issues to 19 (only cyclop complexity issues remain)
anandkuma77
force-pushed
the
cm-902-golangci-lint-secondpart
branch
from
16f1a42 to
8bcb9bd
Compare
Contributor
|
@anandkuma77: 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.
What:
Enable additional golangci-lint rules (dupl, err113, gocognit, maintidx, revive, cyclop) and refactor the operator/controller code to satisfy them by reducing complexity, removing duplicated reconciliation paths, and standardizing error handling with static wrapped errors.
Why:
This improves code quality and maintainability by enforcing stricter lint standards, making reconciliation logic easier to reason about, and ensuring consistent error semantics across the codebase while preserving existing behavior.
Summary by CodeRabbit
Refactor
Chores