Skip to content

fix: guard against nil spec.replicas in deployment analyzer#1683

Open
anxkhn wants to merge 1 commit into
k8sgpt-ai:mainfrom
anxkhn:fix/deployment-nil-replicas
Open

fix: guard against nil spec.replicas in deployment analyzer#1683
anxkhn wants to merge 1 commit into
k8sgpt-ai:mainfrom
anxkhn:fix/deployment-nil-replicas

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Description

The Deployment analyzer dereferences *deployment.Spec.Replicas without a nil
check. Spec.Replicas is a *int32; although the API server usually defaults
it to 1, a Deployment object whose spec.replicas is explicitly unset (nil)
panics the analyze run with a nil pointer dereference at
pkg/analyzer/deployment.go:57.

This guards the comparison with a nil check, mirroring the sibling StatefulSet
analyzer (pkg/analyzer/statefulset.go:97), which already checks
Spec.Replicas != nil before dereferencing. The single guard also protects the
subsequent *deployment.Spec.Replicas reads on lines 58, 62, and 79, since they
only run inside the guarded block.

Added a regression test, TestDeploymentAnalyzerNilReplicas, that analyzes a
Deployment with nil spec.replicas and asserts Analyze returns no results and
does not panic.

Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

Additional Information

No behavior change for Deployments that set spec.replicas (the overwhelming
common case, since the API server defaults it). The only difference is that a
Deployment with an explicitly-unset replicas field is now skipped instead of
crashing the analyzer.

The Deployment analyzer dereferenced *deployment.Spec.Replicas without a
nil check. Spec.Replicas is a *int32 and, although the API server usually
defaults it to 1, a Deployment object whose replicas field is explicitly
unset (nil) panics the analyze run with a nil pointer dereference.

Guard the comparison with a nil check, mirroring the sibling StatefulSet
analyzer which already checks Spec.Replicas != nil before dereferencing.
Add a regression test that analyzes a Deployment with nil Spec.Replicas
and asserts Analyze does not panic.

Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
@anxkhn anxkhn requested review from a team as code owners July 6, 2026 12:46
@github-project-automation github-project-automation Bot moved this to Proposed in Backlog Jul 6, 2026
@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 45.95%. Comparing base (173e4dc) to head (beca44d).
⚠️ Report is 253 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1683       +/-   ##
===========================================
+ Coverage   34.76%   45.95%   +11.18%     
===========================================
  Files          94      121       +27     
  Lines        6342     7533     +1191     
===========================================
+ Hits         2205     3462     +1257     
+ Misses       4046     3838      -208     
- Partials       91      233      +142     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AlexsJones AlexsJones left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the diff — this is a correct, minimal fix. ✅

  • pkg/analyzer/deployment.go:57 did dereference *deployment.Spec.Replicas unconditionally, and Spec.Replicas is a *int32, so a Deployment with an explicitly-unset spec.replicas would panic the analyze run. Real bug.
  • The guard deployment.Spec.Replicas != nil && … short-circuits before the deref, and every other *deployment.Spec.Replicas read (the Status.Replicas > *Spec.Replicas comparison and both failure messages) lives inside that same block, so the single guard covers all of them.
  • It mirrors the sibling StatefulSetAnalyzer, which already uses sts.Spec.Replicas != nil && *(sts.Spec.Replicas) != … — so this keeps the two analyzers consistent.
  • TestDeploymentAnalyzerNilReplicas is a proper regression test: it feeds a nil-replicas Deployment and asserts no panic / zero results. CI (build, golangci-lint, validate, codecov) is green.

No behavior change for the common case where spec.replicas is set. Recommending this for merge — over to @AlexsJones for the final call.


🤝 Handled by Alex's Repo Steward — replies reviewed by @AlexsJones.

Learn more or run your own: https://github.com/AlexsJones/repo-steward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Proposed

Development

Successfully merging this pull request may close these issues.

2 participants