CM-902: Fix golangci-lint errors #368
Conversation
anandkuma77
commented
Jan 29, 2026
anandkuma77
commented
- Refactor TestCreateOrApplyDeployments: extract complex updateIstioCSR functions into helper functions
- Refactor TestCreateOrApplyRBACResource: extract helper functions for status updates
- Refactor TestProcessReconcileRequest: already had helper functions, verified complexity is reduced
- Enable gocyclo linter in .golangci.yaml
- All test cases and coverage preserved, no functionality changed
- Refactor TestCreateOrApplyDeployments: extract complex updateIstioCSR functions into helper functions - Refactor TestCreateOrApplyRBACResource: extract helper functions for status updates - Refactor TestProcessReconcileRequest: already had helper functions, verified complexity is reduced - Enable gocyclo linter in .golangci.yaml - All test cases and coverage preserved, no functionality changed
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. |
openshift-merge-robot
added
the
needs-rebase
WalkthroughBroad refactor across deployment and IstioCSR controllers and operator startup: adds many internal helpers, sentinel errors, logging verbosity constants, consolidates RBAC and resource reconciliation flows, refactors cloud-credentials handling with tests, and enables additional linters in Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[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 |
- Enable dupl linter in .golangci.yaml - Refactor duplicate code in rbacs.go by extracting common reconciliation logic - Created reconcileRBACResource() generic helper function - Created reconcileRole() and reconcileRoleBinding() wrapper functions - Removed duplication between createOrApplyRoles/createOrApplyRoleForLeases - Removed duplication between createOrApplyRoleBindings/createOrApplyRoleBindingForLeases - All tests passing after refactoring
- Enable maintidx linter in golangci-lint configuration - Fix TestMergePodScheduling maintainability by extracting helper functions for creating tolerations, node selectors, and scheduling objects - Fix TestCreateOrApplyDeployments maintainability by extracting inline functions into named wrapper functions - All test cases and code coverage preserved
- Enable err113 linter in golangci-lint configuration
- Replace all dynamic fmt.Errorf() calls with wrapped static errors
- Add static error variables at package level for better error handling
- Fix 40 err113 errors across 12 files:
- deployment: cert_manager_networkpolicy, credentials_request,
deployment_helper, deployment_overrides_validation
- istiocsr: certificates, controller, deployments, install_instiocsr_test,
rbacs, utils
- operator: operatorclient, optional_informer_test
- Improve error handling consistency and enable better error checking
with errors.Is() and errors.As()
- Enable 'mnd' linter in .golangci.yaml - Replace magic numbers with named constants: - Add defaultVolumeMode constant (420) for file permission mode - Add log verbosity level constants (logVerbosityLevelDebug=4, logVerbosityLevelInfo=2) - Add expectedLabelParts constant (2) for label format validation - Add test constants (testFakeRecorderBufferSize, testCertificateDuration, testRenewBeforeDuration) - Use existing DefaultRSAPrivateKeySize constant for RSA key size - Fix all 13 magic number issues across multiple files
- Enable 'cyclop' linter in .golangci.yaml - Refactor high complexity functions by extracting helper functions: - Deployment controller: withCloudCredentials, validation hooks - IstioCSR certificates: updateCertificateParams split into focused helpers - IstioCSR controller: SetupWithManager and processReconcileRequest refactored - IstioCSR deployments: createOrApplyDeployments simplified - IstioCSR install: reconcileIstioCSRDeployment extracted resource loop - IstioCSR RBAC: createOrApplyClusterRoles/Bindings extracted find/reconcile logic - IstioCSR utils: hasObjectChanged, deploymentSpecModified, disallowMultipleIstioCSRInstances refactored - Operator client: ApplyOperatorStatus split into focused helpers - Operator starter: RunOperator extracted initialization and setup logic - Fix error message format in deployments.go to match test expectations - Fix multiple instance check logic to properly stop processing when detected - All unit tests passing after refactoring
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/operator/optionalinformer/optional_informer_test.go (1)
53-60:⚠️ Potential issue | 🟡 MinorStatic analysis still flags this as a dynamic error (err113).
The change from
fmt.Errorftoerrors.Newdoesn't satisfy the err113 linter. To fully address the lint error, define a package-level sentinel error.Proposed fix
type alwaysErrorFakeDiscovery struct { fakediscovery.FakeDiscovery } + +var errExpectedFoo = errors.New("expected foo error") // ServerResourcesForGroupVersion is the only func that OptionalInformer's discovery client calls. func (f *alwaysErrorFakeDiscovery) ServerResourcesForGroupVersion(groupVersion string) (*metav1.APIResourceList, error) { - return nil, errors.New("expected foo error") + return nil, errExpectedFoo }pkg/controller/deployment/deployment_overrides_validation.go (1)
120-127:⚠️ Potential issue | 🟡 MinorWrong error sentinel used for environment variable validation.
The
validateEnvfunction useserrUnsupportedArgwhen reporting unsupported environment variables. This should use a dedicated error variable (e.g.,errUnsupportedEnv) for semantic correctness and clearer error messages.Proposed fix
Add a new error variable and update the validation:
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") errUnsupportedResourceLimit = errors.New("validation failed due to unsupported resource limits") errUnsupportedResourceReq = errors.New("validation failed due to unsupported resource requests") )validateEnv := func(argMap map[string]corev1.EnvVar, supportedEnv []string) error { for k, v := range argMap { if !slices.Contains(supportedEnv, k) { - return fmt.Errorf("%w: %q=%q", errUnsupportedArg, k, v) + return fmt.Errorf("%w: %q=%q", errUnsupportedEnv, k, v) } } return nil }
🤖 Fix all issues with AI agents
In `@pkg/controller/deployment/credentials_request.go`:
- Around line 134-150: The function applyCloudCredentialsToDeployment currently
indexes deployment.Spec.Template.Spec.Containers[0] without guarding against an
empty slice; update it to first check
len(deployment.Spec.Template.Spec.Containers) == 0 and if so append a new
corev1.Container{} (or return an error/log and skip mutation) before accessing
Containers[0], then proceed to append the VolumeMount and Env (respecting the
existing envVar nil check); ensure you reference
deployment.Spec.Template.Spec.Containers, VolumeMounts and Env when making the
defensive check and modification.
In `@pkg/controller/istiocsr/controller.go`:
- Around line 134-189: The linter wants unexported helpers placed after the
exported Reconcile method; move the methods createReconcileMapFunc,
extractNamespaceFromLabels, and isObjectOfInterest so they are defined below the
Reconcile(...) method in the same file, keeping their signatures and logic
unchanged and updating any references if necessary so Reconcile can call them as
before.
- Around line 181-185: The r.log.Error call in controller.go is using a
printf-style message with positional args; update the r.log.Error invocation in
the block that checks key/expectedLabelParts (variables: key,
expectedLabelParts) to pass a short message and then structured key/value pairs
(e.g. "invalid label format", "error", errInvalidLabelFormat, "label",
IstiocsrResourceWatchLabelName, "value", value, "resource", obj.GetName()) so
the logr.Logger receives even-numbered key/value pairs after the message; ensure
you remove the formatted string and positional args and replace them with the
structured pairs in the r.log.Error call.
In `@pkg/controller/istiocsr/deployments.go`:
- Around line 29-33: Remove the unused sentinel error variable
errKeyNotFoundInConfigMap from the var block (alongside errIstioCSRImageNotSet,
errFailedToFetchCACertificate, errFailedToFindCACertificate) so golangci-lint
unused check passes; simply delete the declaration of errKeyNotFoundInConfigMap
and run `go vet`/`golangci-lint` to verify no other references exist.
In `@pkg/controller/istiocsr/utils.go`:
- Around line 505-515: In checkForMultipleInstances replace the dynamic error
construction using errors.New(statusMessage) with fmt.Errorf to satisfy err113;
specifically, pass fmt.Errorf(statusMessage) (or fmt.Errorf("%s",
statusMessage)) into NewMultipleInstanceError so the call becomes
NewMultipleInstanceError(fmt.Errorf(statusMessage)), keeping the existing
statusMessage and NewMultipleInstanceError usage intact.
- Around line 27-34: The declared sentinel
errFailedToCreateIstioCSRWithFinalizersAdded is unused; update the addFinalizer
function to wrap the actual creation error with this sentinel instead of
constructing a new fmt.Errorf string directly (use fmt.Errorf with %w to wrap,
e.g. wrap the error returned from the client Create call with
errFailedToCreateIstioCSRWithFinalizersAdded), or if you prefer the other option
remove the errFailedToCreateIstioCSRWithFinalizersAdded declaration to avoid the
unused variable; reference symbols: errFailedToCreateIstioCSRWithFinalizersAdded
and addFinalizer.
- Around line 489-493: The code dynamically creates an error with
errors.New(statusMessage) which violates err113; define a package-level sentinel
error (e.g., var ErrInstanceFailed = errors.New("instance failed")) and replace
the dynamic creation by wrapping that sentinel with the statusMessage using
fmt.Errorf with %w (so the original message is preserved but the error is a
static sentinel), then pass the wrapped error into utilerrors.NewAggregate
alongside updateErr in the NewMultipleInstanceError call; update references
around NewMultipleInstanceError, utilerrors.NewAggregate, statusMessage, and
updateErr accordingly.
In `@pkg/operator/operatorclient/operatorclient.go`:
- Around line 62-69: The linter flagged ordering: move the unexported helper
methods (applyStatusForNewInstance, applyStatusForExistingInstance,
extractOperatorStatus, setConditionTransitionTimes, canonicalizeStatuses,
isStatusUnchanged, applyStatus) so they appear after the last exported method
RemoveFinalizer and before saveFinalizers; reorder the file accordingly to place
all those unexported functions below RemoveFinalizer to satisfy funcorder rules
without changing their signatures or behavior.
- Around line 143-148: Remove the unused operatorStatus parameter from the
prepareStatusForApply signature and all call sites; update func
prepareStatusForApply(desired *applyconfig.CertManagerApplyConfiguration,
desiredConfiguration *applyoperatorv1.OperatorStatusApplyConfiguration) to drop
operatorStatus, and modify any callers (e.g., the call that currently passes
operatorStatus around) to call prepareStatusForApply with only the desired and
desiredConfiguration arguments; ensure you update imports/types if necessary to
match the new parameter list.
In `@pkg/operator/starter.go`:
- Around line 209-222: The startIstioCSRController currently calls
ctrl.SetupSignalHandler() directly and blocks; change its signature to accept a
parent context (e.g., ctx context.Context) so it uses the operator's root
cancellation instead of creating a new signal handler; after creating the
manager via NewControllerManager(), start the manager with manager.Start(ctx)
but do so in a goroutine (since Start blocks) and surface startup errors via
logging or a channel (or return immediately and log errors) so callers like
RunOperator can pass in the root ctx and control shutdown consistently; remove
any direct calls to ctrl.SetupSignalHandler() from startIstioCSRController.
- Around line 69-71: The call to startIstioCSRController creates its own signal
handler (ctrl.SetupSignalHandler) causing inconsistent shutdown; change the call
site to pass the parent ctx into startIstioCSRController(ctx) and modify the
function signature startIstioCSRController to accept a context.Context
parameter, then replace ctrl.SetupSignalHandler() usage inside
startIstioCSRController with that passed ctx (use ctx.Done() or context-aware
helpers for controller manager lifecycle and informer stops) so the IstioCSR
controller honors cancellation of the parent context.
🧹 Nitpick comments (2)
pkg/operator/starter.go (1)
86-104: Wrap client initialization errors for better diagnostics.Static analysis flags that errors from external packages should be wrapped. This provides better context when troubleshooting initialization failures.
♻️ Proposed fix
kubeClient, err := kubernetes.NewForConfig(cc.ProtoKubeConfig) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create kubernetes client: %w", err) } certManagerOperatorClient, err := certmanoperatorclient.NewForConfig(cc.KubeConfig) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create cert-manager operator client: %w", err) } apiExtensionsClient, err := apiextensionsclient.NewForConfig(cc.KubeConfig) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create api extensions client: %w", err) } configClient, err := configv1client.NewForConfig(cc.KubeConfig) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create config client: %w", err) }pkg/controller/deployment/deployment_helper.go (1)
243-243: Consider applying the sentinel error pattern consistently.The functions
getOverrideReplicasFor,getOverrideResourcesFor, andgetOverrideSchedulingForstill use inline error strings (fmt.Errorf("unsupported deployment name %q provided", ...)) whilegetOverrideArgsFor,getOverrideEnvFor, andgetOverridePodLabelsForwere updated to use the newerrUnsupportedDeploymentNamesentinel.For consistency and to fully satisfy
err113, consider updating these three functions as well.♻️ Proposed fix for consistency
default: - return nil, fmt.Errorf("unsupported deployment name %q provided", deploymentName) + return nil, fmt.Errorf("%w: %q", errUnsupportedDeploymentName, deploymentName) }Apply similar changes to lines 270 and 297.
Also applies to: 270-270, 297-297
| func (c OperatorClient) applyStatusForNewInstance(ctx context.Context, fieldManager string, desired *applyconfig.CertManagerApplyConfiguration, desiredConfiguration *applyoperatorv1.OperatorStatusApplyConfiguration) error { | ||
| v1helpers.SetApplyConditionsLastTransitionTime(c.Clock, &desiredConfiguration.Conditions, nil) | ||
| desiredStatus := &applyconfig.CertManagerStatusApplyConfiguration{ | ||
| OperatorStatusApplyConfiguration: *desiredConfiguration, | ||
| } | ||
| desired.WithStatus(desiredStatus) | ||
| return c.applyStatus(ctx, fieldManager, desired) | ||
| } |
There was a problem hiding this comment.
Move unexported helper methods after all exported methods.
The funcorder linter flagged all these new unexported methods (applyStatusForNewInstance, applyStatusForExistingInstance, extractOperatorStatus, setConditionTransitionTimes, canonicalizeStatuses, isStatusUnchanged, applyStatus) for being placed before exported methods. Per Go conventions enforced by the linter, they should be placed after RemoveFinalizer (the last exported method) and before saveFinalizers.
Since this PR specifically aims to fix golangci-lint errors, these funcorder violations should be addressed.
Also applies to: 71-91, 93-107, 109-118, 120-123, 125-141, 150-159
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 62-62: unexported method "applyStatusForNewInstance" for struct "OperatorClient" should be placed after the exported method "RemoveFinalizer"
(funcorder)
🤖 Prompt for AI Agents
In `@pkg/operator/operatorclient/operatorclient.go` around lines 62 - 69, The
linter flagged ordering: move the unexported helper methods
(applyStatusForNewInstance, applyStatusForExistingInstance,
extractOperatorStatus, setConditionTransitionTimes, canonicalizeStatuses,
isStatusUnchanged, applyStatus) so they appear after the last exported method
RemoveFinalizer and before saveFinalizers; reorder the file accordingly to place
all those unexported functions below RemoveFinalizer to satisfy funcorder rules
without changing their signatures or behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.golangci.yaml (1)
112-114:⚠️ Potential issue | 🔴 CriticalFix typo in linter exclusion path: change
pkg/operatorstopkg/operator.The exclusion rule targets
pkg/operators(plural), but the actual directory ispkg/operator(singular). This causes the rule to be ineffective. If the intent is to exclude linters from the operator package (which contains auto-generated clientset, informers, and lister code alongside hand-written setup/starter code), correct the path topkg/operator.pkg/controller/istiocsr/rbacs.go (3)
166-177:⚠️ Potential issue | 🟡 MinorSilent failure: empty name is logged but not returned as an error.
When both
desired.GetName()andexisting.GetName()are empty, the function logs an error (line 172) but continues to update the status with an emptyClusterRolename and returnsnil(assumingupdateStatussucceeds). This silent failure could mask issues and cause confusion in subsequent reconciliation cycles.Consider returning an error when the name cannot be determined:
🔧 Suggested 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(errErrorUpdatingClusterRoleName, "istiocsr", istiocsr.GetNamespace()) + return "", errErrorUpdatingClusterRoleName } } istiocsr.Status.ClusterRole = name return name, r.updateStatus(r.ctx, istiocsr) }
278-289:⚠️ Potential issue | 🟡 MinorSame silent failure pattern as
updateClusterRoleNameInStatus.This function has the same issue: when the name cannot be determined, an error is logged (line 284) but the status is updated with an empty name and no error is returned. Apply the same fix to return the error instead of just logging it.
1-13:⚠️ Potential issue | 🔴 CriticalDefine missing
testErrorvariable in test files.
testErroris referenced in 40+ locations across multiple test files (rbacs_test.go,services_test.go,deployments_test.go,controller_test.go,certificates_test.go,install_instiocsr_test.go) but is never defined anywhere in the codebase. This will cause a compilation failure. DefinetestErroras a package-level variable (e.g., in a test helper file or at the top of the test files).
🤖 Fix all issues with AI agents
In `@pkg/controller/istiocsr/controller.go`:
- Around line 331-346: The Ready condition in
Reconciler.handleIrrecoverableError is being set with v1alpha1.ReasonReady which
is inconsistent for an irrecoverable failure; change the call that sets Ready
(the second SetCondition on istiocsr.Status) to use v1alpha1.ReasonFailed (same
reason as Degraded) so Ready=False has a failure reason consistent with the
Degraded condition.
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 335-349: The current logic logs "resource already exists and is in
expected state" when exist==false because the else matches the combined
condition; change the control flow to first check exist and then handle
update/unchanged, otherwise handle creation so logs match reality: use if exist
{ if hasObjectChanged(desired,fetched) { call r.UpdateWithRetry(...);
r.eventRecorder.Eventf(...) } else { r.log.V(...).Info(...) } } else { call
r.Create(r.ctx, desired); r.eventRecorder.Eventf(...) } and keep the same error
handling via FromClientError for UpdateWithRetry and Create.
🧹 Nitpick comments (4)
.golangci.yaml (1)
32-32: Consider whether bothgocycloandcyclopare needed.Both
gocyclo(line 32) andcyclop(line 66) measure cyclomatic complexity. Having both enabled may produce duplicate or overlapping warnings for the same functions. If this is intentional (e.g., different thresholds or reporting preferences), feel free to ignore.Also applies to: 66-66
pkg/controller/istiocsr/deployments.go (1)
88-95: Inconsistent logging verbosity constant usage.Line 89 uses
V(1)directly while other debug-level logs in this file (lines 57, 82, 612) uselogVerbosityLevelDebug. Consider using the constant for consistency.♻️ Suggested fix
func (r *Reconciler) updateDeploymentResource(istiocsr *v1alpha1.IstioCSR, desired *appsv1.Deployment, deploymentName string) error { - r.log.V(1).Info("deployment has been modified, updating to desired state", "name", deploymentName) + r.log.V(logVerbosityLevelDebug).Info("deployment has been modified, updating to desired state", "name", deploymentName) if err := r.UpdateWithRetry(r.ctx, desired); err != nil {pkg/controller/istiocsr/rbacs.go (1)
91-103: ClusterRole is cluster-scoped; namespace references are misleading.
ClusterRoleis a cluster-scoped resource, sodesired.GetNamespace()always returns an empty string. TheroleNameformatnamespace/nameand theNamespacefield inObjectKeyare misleading. While this doesn't break functionality (the namespace is ignored for cluster-scoped lookups), it reduces code clarity.Consider simplifying to just use the name:
♻️ Suggested improvement
func (r *Reconciler) findClusterRoleByStatus(istiocsr *v1alpha1.IstioCSR, desired *rbacv1.ClusterRole) (bool, *rbacv1.ClusterRole, string, error) { - roleName := fmt.Sprintf("%s/%s", desired.GetNamespace(), istiocsr.Status.ClusterRole) + roleName := istiocsr.Status.ClusterRole fetched := &rbacv1.ClusterRole{} key := client.ObjectKey{ - Name: istiocsr.Status.ClusterRole, - Namespace: desired.GetNamespace(), + Name: istiocsr.Status.ClusterRole, }pkg/operator/operatorclient/operatorclient.go (1)
62-69: Add nil-safe wrapper for Clock to prevent panics.The
Clockfield inOperatorClientcan theoretically be nil if instantiated without initialization, and it's dereferenced inapplyStatusForNewInstance(line 63) andsetConditionTransitionTimes(lines 114, 116) when passed toSetApplyConditionsLastTransitionTime, which callsclock.Now(). While current production code paths properly initializeClockviainitializeOperatorComponents, consider adding defensive safeguards to prevent panics from future direct struct instantiations or misconfigurations.Optional safeguard
+func (c OperatorClient) effectiveClock() clock.PassiveClock { + if c.Clock == nil { + return clock.RealClock{} + } + return c.Clock +} + func (c OperatorClient) applyStatusForNewInstance(ctx context.Context, fieldManager string, desired *applyconfig.CertManagerApplyConfiguration, desiredConfiguration *applyoperatorv1.OperatorStatusApplyConfiguration) error { - v1helpers.SetApplyConditionsLastTransitionTime(c.Clock, &desiredConfiguration.Conditions, nil) + v1helpers.SetApplyConditionsLastTransitionTime(c.effectiveClock(), &desiredConfiguration.Conditions, nil) desiredStatus := &applyconfig.CertManagerStatusApplyConfiguration{ OperatorStatusApplyConfiguration: *desiredConfiguration, } desired.WithStatus(desiredStatus) return c.applyStatus(ctx, fieldManager, desired) } @@ func (c OperatorClient) setConditionTransitionTimes(desiredConfiguration, operatorStatus *applyoperatorv1.OperatorStatusApplyConfiguration) { if desiredConfiguration.Conditions == nil { return } if operatorStatus != nil { - v1helpers.SetApplyConditionsLastTransitionTime(c.Clock, &desiredConfiguration.Conditions, operatorStatus.Conditions) + v1helpers.SetApplyConditionsLastTransitionTime(c.effectiveClock(), &desiredConfiguration.Conditions, operatorStatus.Conditions) } else { - v1helpers.SetApplyConditionsLastTransitionTime(c.Clock, &desiredConfiguration.Conditions, nil) + v1helpers.SetApplyConditionsLastTransitionTime(c.effectiveClock(), &desiredConfiguration.Conditions, nil) } }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/istiocsr/utils.go (1)
312-396:⚠️ Potential issue | 🟠 MajorAdd nil-pointer and empty-container guards to deployment/container checks.
Several helpers dereference pointers or index into slices without guards.
Replicasis an optional*int32in Kubernetes specs and can be nil;ReadinessProbeandHTTPGetare optional pointers and can be nil;SecurityContextis optional and can be nil. The containers slice can also be empty, causingContainers[0]to panic even thoughcheckDeploymentTemplateonly guards against differing lengths, not zero-length slices. Add nil and length checks before dereferencing.🛡️ Suggested nil-safe guards
func checkDeploymentBasicSpec(desired, fetched *appsv1.Deployment) bool { - return *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 { + if desired.Spec.Replicas != fetched.Spec.Replicas { + return true + } + } else if *desired.Spec.Replicas != *fetched.Spec.Replicas { + return true + } + return !reflect.DeepEqual(desired.Spec.Selector.MatchLabels, fetched.Spec.Selector.MatchLabels) } func checkDeploymentContainer(desired, fetched *appsv1.Deployment) bool { + if len(desired.Spec.Template.Spec.Containers) == 0 || len(fetched.Spec.Template.Spec.Containers) == 0 { + return len(desired.Spec.Template.Spec.Containers) != len(fetched.Spec.Template.Spec.Containers) + } desiredContainer := desired.Spec.Template.Spec.Containers[0] fetchedContainer := fetched.Spec.Template.Spec.Containers[0] if checkContainerBasicFields(desiredContainer, fetchedContainer) { return true } if checkContainerPorts(desiredContainer, fetchedContainer) { return true } if checkContainerProbe(desiredContainer, fetchedContainer) { return true } return checkContainerResources(desiredContainer, fetchedContainer) } func checkContainerProbe(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 checkContainerResources(desired, fetched corev1.Container) bool { return !reflect.DeepEqual(desired.Resources, fetched.Resources) || - !reflect.DeepEqual(*desired.SecurityContext, *fetched.SecurityContext) || + !reflect.DeepEqual(desired.SecurityContext, fetched.SecurityContext) || !reflect.DeepEqual(desired.VolumeMounts, fetched.VolumeMounts) }
🤖 Fix all issues with AI agents
In `@pkg/controller/istiocsr/utils.go`:
- Around line 267-279: In checkRBACObjectSpecModified, avoid forced type
assertions on fetched; replace fetched.(*rbacv1.ClusterRoleBinding) and
fetched.(*rbacv1.RoleBinding) with ok-assert checks (e.g., v, ok :=
fetched.(*rbacv1.ClusterRoleBinding); if !ok { /* handle: return nil or panic
with clear message */ }) before calling checkClusterRoleBindingModified and
checkRoleBindingModified, or explicitly panic with a clear message if you
prefer; ensure you return a pointer to the bool result as the function currently
does.
- Fix forced type assertions in utils.go with ok-assert pattern - Fix misleading log messages when resources don't exist (certificates, services, networkpolicies, rbacs, deployments) - Fix logr.Logger.Error to use structured key/value pairs - Fix funcorder: move unexported methods below Reconcile - Fix Ready condition reason for irrecoverable errors (use ReasonFailed) - Add safety check for empty containers in credentials_request.go - Remove unused error variable errKeyNotFoundInConfigMap - Use sentinel errors instead of dynamic error creation - Remove unused operatorStatus parameter - Pass context to startIstioCSRController for consistent shutdown behavior
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)
166-176:⚠️ Potential issue | 🟡 MinorFix logr.Logger.Error calls with odd number of key/value arguments and non-descriptive messages.
Lines 172 and 284 pass invalid arguments to
logr.Logger.Error. The calls use a single key-value pair as arguments after the error, but treat the key as the message, violating the API signature:Error(err error, msg string, keysAndValues ...interface{}). The keysAndValues args must be even-length key-value pairs.Proposed fix
- r.log.Error(errErrorUpdatingClusterRoleName, "istiocsr", istiocsr.GetNamespace()) + r.log.Error(errErrorUpdatingClusterRoleName, "failed to resolve clusterrole name for istiocsr", + "namespace", istiocsr.GetNamespace(), + "name", istiocsr.GetName()) ... - r.log.Error(errErrorUpdatingClusterRoleBindingName, "istiocsr", istiocsr.GetNamespace()) + r.log.Error(errErrorUpdatingClusterRoleBindingName, "failed to resolve clusterrolebinding name for istiocsr", + "namespace", istiocsr.GetNamespace(), + "name", istiocsr.GetName())
🤖 Fix all issues with AI agents
In `@pkg/controller/deployment/credentials_request.go`:
- Around line 136-139: Define a package-level sentinel error (e.g. var
ErrNoContainers = errors.New("no containers")) and replace the dynamic
fmt.Errorf return in applyCloudCredentialsToDeployment with a wrapped sentinel,
e.g. return fmt.Errorf("deployment %s/%s has no containers: %w",
deployment.GetNamespace(), deployment.GetName(), ErrNoContainers); add the
"errors" import if needed so golangci-lint err113 is fixed.
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 105-121: The two functions findClusterRoleByLabels and
findClusterRoleBindingByLabels share almost identical logic; either refactor to
a single parameterized helper (e.g., findResourceByLabels[T client.Object] or
findByLabels(ctx, listObj client.ObjectList, desired client.Object, ...) that
accepts the list/result types and constructs the fetched object) and centralize:
call r.List with MatchingLabels, handle zero/one/multiple items, DeepCopyInto
the fetched object, emit the same eventRecorder.Eventf for duplicates, and
return the same tuple shape; or if refactoring is infeasible, add a
//nolint:dupl comment above both functions with a brief explanation why the
duplication is type-specific and cannot be shared. Ensure the helper preserves
existing behaviors (error wrapping via FromClientError, NewIrrecoverableError
with errMultipleClusterRolesExist, and roleName formatting) and update callers
to use the new helper.
In `@pkg/controller/istiocsr/utils.go`:
- Around line 389-393: The checkContainerProbe function can panic if
ReadinessProbe or ReadinessProbe.HTTPGet is nil; update checkContainerProbe to
perform nil-safe comparisons: first handle cases where one ReadinessProbe is nil
and the other is not (consider that a difference), then if both are non-nil
compare InitialDelaySeconds and PeriodSeconds, and separately handle HTTPGet
being nil on either side (consider nil vs non-nil a difference) before comparing
HTTPGet.Path; use the container parameter names desired and fetched to locate
the logic and ensure all dereferences of ReadinessProbe and
ReadinessProbe.HTTPGet are guarded.
- Around line 395-399: The checkContainerResources function can panic by
dereferencing SecurityContext without nil checks; update it to compare
SecurityContext safely (e.g., use reflect.DeepEqual on the pointers themselves
or check for nil before dereferencing) instead of *desired.SecurityContext and
*fetched.SecurityContext, keeping the existing comparisons for Resources and
VolumeMounts; ensure you reference the checkContainerResources function and the
desired.SecurityContext / fetched.SecurityContext symbols so the fix safely
handles cases where either pointer is nil.
🧹 Nitpick comments (2)
pkg/operator/operatorclient/operatorclient.go (1)
93-107: JSON round-trip for status extraction adds overhead.The
extractOperatorStatusfunction uses JSON marshal/unmarshal to copy the operator status. While this works, it's less efficient than a direct field copy or usingDeepCopyif available on the type.pkg/controller/istiocsr/certificates.go (1)
23-57: Cyclomatic complexity slightly exceeds limit.The
createOrApplyCertificatesfunction has cyclomatic complexity of 11 (limit is 10). Consider extracting the update-or-create logic into a helper similar todeployments.go.♻️ Proposed refactor to reduce complexity
func (r *Reconciler) createOrApplyCertificates(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { desired, err := r.getCertificateObject(istiocsr, resourceLabels) if err != nil { return fmt.Errorf("failed to generate certificate resource for creation in %s: %w", istiocsr.GetNamespace(), err) } certificateName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) r.log.V(logVerbosityLevelDebug).Info("reconciling certificate resource", "name", certificateName) fetched := &certmanagerv1.Certificate{} exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched) if err != nil { return FromClientError(err, "failed to check %s certificate resource already exists", certificateName) } + return r.reconcileCertificateResource(istiocsr, desired, fetched, certificateName, exist, istioCSRCreateRecon) +} + +func (r *Reconciler) reconcileCertificateResource(istiocsr *v1alpha1.IstioCSR, desired, fetched *certmanagerv1.Certificate, certificateName string, exist, istioCSRCreateRecon bool) error { if exist && istioCSRCreateRecon { r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s certificate resource already exists, maybe from previous installation", certificateName) } if exist && hasObjectChanged(desired, fetched) { r.log.V(1).Info("certificate has been modified, updating to desired state", "name", certificateName) if err := r.UpdateWithRetry(r.ctx, desired); err != nil { return FromClientError(err, "failed to update %s certificate resource", certificateName) } r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "certificate resource %s reconciled back to desired state", certificateName) - } else if exist { + return nil + } + if exist { r.log.V(logVerbosityLevelDebug).Info("certificate resource already exists and is in expected state", "name", certificateName) + return nil } - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return FromClientError(err, "failed to create %s certificate resource", certificateName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "certificate resource %s created", certificateName) + if err := r.Create(r.ctx, desired); err != nil { + return FromClientError(err, "failed to create %s certificate resource", certificateName) } - + r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "certificate resource %s created", certificateName) return nil }
- Fix err113: Add sentinel errors (errCloudSecretNotFound, errDeploymentHasNoContainers) and wrap them with context in credentials_request.go - Fix dupl: Extract generic helper function findRBACResourceByLabels to eliminate duplication between findClusterRoleByLabels and findClusterRoleBindingByLabels - Fix nil pointer dereferences: Add nil checks for ReadinessProbe, HTTPGet, and SecurityContext in checkContainerProbe and checkContainerResources functions
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/utils.go (1)
370-387:⚠️ Potential issue | 🟡 MinorCompare full port specification, not just container port number.
The current comparison only checks
ContainerPort; changes toName,Protocol, orHostPortwon't be detected. While the managed deployments don't currently set these fields, usereflect.DeepEqualto handle potential future changes and align with similar reconciliation patterns elsewhere in the codebase.Proposed fix
func checkContainerPorts(desired, fetched corev1.Container) bool { if len(desired.Ports) != len(fetched.Ports) { return true } for _, fetchedPort := range fetched.Ports { matched := false for _, desiredPort := range desired.Ports { - if fetchedPort.ContainerPort == desiredPort.ContainerPort { + if reflect.DeepEqual(fetchedPort, desiredPort) { matched = true break } } if !matched { return true } } return false }
🤖 Fix all issues with AI agents
In `@pkg/controller/deployment/credentials_request_test.go`:
- Around line 182-271: The test's nested closures increase cognitive complexity
and incorrectly access the embedded VolumeSource secret field; extract the
inline checkVolume and checkEnvVar closures into package-level helper functions
(e.g., verifyCloudVolume(t *testing.T, vol *corev1.Volume, expectedSecret
string) and verifyCloudEnvVar(t *testing.T, env *corev1.EnvVar, expectEnv bool,
expectedName string)) and update selectors to use the embedded field access
(replace vol.VolumeSource.Secret.SecretName with vol.Secret.SecretName). Update
the table-driven tests to call these helpers (for AWS/GCP cases) and keep the
existing assertions around createCloudCredentialsResources,
cloudCredentialsVolumeName and errUnsupportedCloudProvider unchanged.
- Around line 77-180: The test TestApplyCloudCredentialsToDeployment is nested
too deeply; refactor the subtest body to use early returns so the success
verification path isn't inside the else block. Specifically, after calling
applyCloudCredentialsToDeployment in the t.Run closure, handle the
expected-error case immediately (if tt.wantErr != nil: assert error matches and
return), then continue with the success assertions (volume, volumeMount, env
var) without an else branch; for the env var check, guard with an early return
or a simple if that doesn't add extra nesting. This targets the t.Run anonymous
function inside TestApplyCloudCredentialsToDeployment and preserves all
assertions while reducing nesting to meet the lint rule.
In `@pkg/controller/deployment/credentials_request.go`:
- Around line 85-96: The GCP helper createGCPCredentialsResources always returns
nil for the EnvVar, so remove that unused third return from its signature and
implementation (update createGCPCredentialsResources to return only
(*corev1.Volume, *corev1.VolumeMount)), then update its call site in
createCloudCredentialsResources to accept only two return values and set the
envVar to nil explicitly (i.e., volume, volumeMount :=
createGCPCredentialsResources(secretName); return volume, volumeMount, nil,
nil). Also update any other callers and imports impacted by the signature change
to match the new two-value return.
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 164-193: The reconcile path must copy the Name and ResourceVersion
from the fetched object to the desired object for resources created with
generateName before performing an update: in reconcileClusterRoleResource,
before calling updateClusterRole, set desired.Name = fetched.Name and
desired.ResourceVersion = fetched.ResourceVersion so client.ObjectKeyFromObject
and UpdateWithRetry succeed; apply the same fix where reconcile calls
updateClusterRoleBinding (copy fetched.Name and fetched.ResourceVersion into
desired before calling updateClusterRoleBinding) to handle generateName-created
ClusterRole/ClusterRoleBinding objects.
- Fix unparam: Remove unused EnvVar return from createGCPCredentialsResources GCP helper always returns nil for EnvVar, so removed it from signature and set to nil at call site instead - Fix nestif: Flatten nested conditionals in TestApplyCloudCredentialsResources using early returns to reduce nesting from 6 to 5 levels - Fix cognitive complexity: Extract checkVolume and checkEnvVar closures to package-level helpers and fix embedded field access (vol.Secret.SecretName instead of vol.VolumeSource.Secret.SecretName) - Fix generateName update: Copy Name and ResourceVersion from fetched to desired before update for ClusterRole/ClusterRoleBinding resources created with generateName to prevent UpdateWithRetry failures
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/istiocsr/rbacs.go (1)
212-223:⚠️ Potential issue | 🟡 MinorFix logr.Error key/value pairs to avoid malformed log entries.
Both
Errorcalls at lines 218 and 343 pass an odd number of keysAndValues (a single value with no corresponding key). The logr.Error signature isError(err error, msg string, keysAndValues ...any), which requires key/value pairs. Provide explicit key/value pairs for proper structured logging:Suggested fix
- r.log.Error(errErrorUpdatingClusterRoleName, "istiocsr", istiocsr.GetNamespace()) + r.log.Error(errErrorUpdatingClusterRoleName, "failed to resolve clusterrole name for istiocsr", "namespace", istiocsr.GetNamespace()) @@ - r.log.Error(errErrorUpdatingClusterRoleBindingName, "istiocsr", istiocsr.GetNamespace()) + r.log.Error(errErrorUpdatingClusterRoleBindingName, "failed to resolve clusterrolebinding name for istiocsr", "namespace", istiocsr.GetNamespace())
🤖 Fix all issues with AI agents
In `@pkg/controller/deployment/credentials_request_test.go`:
- Around line 19-21: Update the comment above the helper function
checkVolumeSecretName to satisfy the godot linter by adding a trailing period to
the sentence "Uses embedded field access: vol.Secret.SecretName instead of
vol.VolumeSource.Secret.SecretName" so the comment ends with a period.
| // checkVolumeSecretName verifies that a volume has the expected secret name. | ||
| // Uses embedded field access: vol.Secret.SecretName instead of vol.VolumeSource.Secret.SecretName | ||
| func checkVolumeSecretName(t *testing.T, vol *corev1.Volume, expectedSecretName string) { |
There was a problem hiding this comment.
Fix the godot lint error on the comment punctuation.
Line 20’s comment is missing a trailing period, which trips the godot linter.
Proposed fix
-// Uses embedded field access: vol.Secret.SecretName instead of vol.VolumeSource.Secret.SecretName
+// Uses embedded field access: vol.Secret.SecretName instead of vol.VolumeSource.Secret.SecretName.📝 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.
| // checkVolumeSecretName verifies that a volume has the expected secret name. | |
| // Uses embedded field access: vol.Secret.SecretName instead of vol.VolumeSource.Secret.SecretName | |
| func checkVolumeSecretName(t *testing.T, vol *corev1.Volume, expectedSecretName string) { | |
| // checkVolumeSecretName verifies that a volume has the expected secret name. | |
| // Uses embedded field access: vol.Secret.SecretName instead of vol.VolumeSource.Secret.SecretName. | |
| func checkVolumeSecretName(t *testing.T, vol *corev1.Volume, expectedSecretName string) { |
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 20-20: Comment should end in a period
(godot)
🤖 Prompt for AI Agents
In `@pkg/controller/deployment/credentials_request_test.go` around lines 19 - 21,
Update the comment above the helper function checkVolumeSecretName to satisfy
the godot linter by adding a trailing period to the sentence "Uses embedded
field access: vol.Secret.SecretName instead of
vol.VolumeSource.Secret.SecretName" so the comment ends with a period.
|
@anandkuma77: all tests passed! 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. |
3 participants
