feat: improve HPA analyzer to check ScaleTargetRef resources (#283)

* feat: improve HPA analyzer to check ScaleTargetRef resources

Signed-off-by: Rakshit Gondwal <rakshitgondwal3@gmail.com>

* feat: modify tests

Signed-off-by: Rakshit Gondwal <rakshitgondwal3@gmail.com>

* feat: improve all ScaleTargetRef to check for resources

Signed-off-by: Rakshit Gondwal <rakshitgondwal3@gmail.com>

* test: add test cases for all ScaleTargetRef types

Signed-off-by: Rakshit Gondwal <rakshitgondwal3@gmail.com>

* refactor: use interface to avoid dupplication

Signed-off-by: Matthis Holleville <matthish29@gmail.com>
Signed-off-by: Rakshit Gondwal <rakshitgondwal3@gmail.com>

* test: add test case for NoResourceConfiguredForScaleTargetRef

Signed-off-by: Rakshit Gondwal <rakshitgondwal3@gmail.com>

---------

Signed-off-by: Rakshit Gondwal <rakshitgondwal3@gmail.com>
Signed-off-by: Matthis Holleville <matthish29@gmail.com>
Co-authored-by: Matthis <99146727+matthisholleville@users.noreply.github.com>

pkg/analyzer/hpa.go

@@ -5,6 +5,8 @@ import (
 
 	"github.com/k8sgpt-ai/k8sgpt/pkg/common"
 	"github.com/k8sgpt-ai/k8sgpt/pkg/util"
+	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -30,28 +32,28 @@ func (HpaAnalyzer) Analyze(a common.Analyzer) ([]common.Result, error) {
 
 		// check ScaleTargetRef exist
 		scaleTargetRef := hpa.Spec.ScaleTargetRef
-		scaleTargetRefNotFound := false
+		var podInfo PodInfo
 
 		switch scaleTargetRef.Kind {
 		case "Deployment":
-			_, err := a.Client.GetClient().AppsV1().Deployments(hpa.Namespace).Get(a.Context, scaleTargetRef.Name, metav1.GetOptions{})
+			deployment, err := a.Client.GetClient().AppsV1().Deployments(hpa.Namespace).Get(a.Context, scaleTargetRef.Name, metav1.GetOptions{})
-			if err != nil {
+			if err == nil {
-				scaleTargetRefNotFound = true
+				podInfo = DeploymentInfo{deployment}
 			}
 		case "ReplicationController":
-			_, err := a.Client.GetClient().CoreV1().ReplicationControllers(hpa.Namespace).Get(a.Context, scaleTargetRef.Name, metav1.GetOptions{})
+			rc, err := a.Client.GetClient().CoreV1().ReplicationControllers(hpa.Namespace).Get(a.Context, scaleTargetRef.Name, metav1.GetOptions{})
-			if err != nil {
+			if err == nil {
-				scaleTargetRefNotFound = true
+				podInfo = ReplicationControllerInfo{rc}
 			}
 		case "ReplicaSet":
-			_, err := a.Client.GetClient().AppsV1().ReplicaSets(hpa.Namespace).Get(a.Context, scaleTargetRef.Name, metav1.GetOptions{})
+			rs, err := a.Client.GetClient().AppsV1().ReplicaSets(hpa.Namespace).Get(a.Context, scaleTargetRef.Name, metav1.GetOptions{})
-			if err != nil {
+			if err == nil {
-				scaleTargetRefNotFound = true
+				podInfo = ReplicaSetInfo{rs}
 			}
 		case "StatefulSet":
-			_, err := a.Client.GetClient().AppsV1().StatefulSets(hpa.Namespace).Get(a.Context, scaleTargetRef.Name, metav1.GetOptions{})
+			ss, err := a.Client.GetClient().AppsV1().StatefulSets(hpa.Namespace).Get(a.Context, scaleTargetRef.Name, metav1.GetOptions{})
-			if err != nil {
+			if err == nil {
-				scaleTargetRefNotFound = true
+				podInfo = StatefulSetInfo{ss}
 			}
 		default:
 			failures = append(failures, common.Failure{
@@ -60,7 +62,7 @@ func (HpaAnalyzer) Analyze(a common.Analyzer) ([]common.Result, error) {
 			})
 		}
 
-		if scaleTargetRefNotFound {
+		if podInfo == nil {
 			failures = append(failures, common.Failure{
 				Text: fmt.Sprintf("HorizontalPodAutoscaler uses %s/%s as ScaleTargetRef which does not exist.", scaleTargetRef.Kind, scaleTargetRef.Name),
 				Sensitive: []common.Sensitive{
@@ -70,6 +72,26 @@ func (HpaAnalyzer) Analyze(a common.Analyzer) ([]common.Result, error) {
 					},
 				},
 			})
+		} else {
+			containers := len(podInfo.GetPodSpec().Containers)
+			for _, container := range podInfo.GetPodSpec().Containers {
+				if container.Resources.Requests == nil || container.Resources.Limits == nil {
+					containers--
+				}
+			}
+
+			if containers <= 0 {
+				failures = append(failures, common.Failure{
+					Text: fmt.Sprintf("%s %s/%s does not have resource configured.", scaleTargetRef.Kind, a.Namespace, scaleTargetRef.Name),
+					Sensitive: []common.Sensitive{
+						{
+							Unmasked: scaleTargetRef.Name,
+							Masked:   util.MaskString(scaleTargetRef.Name),
+						},
+					},
+				})
+			}
+
 		}
 
 		if len(failures) > 0 {
@@ -96,3 +118,43 @@ func (HpaAnalyzer) Analyze(a common.Analyzer) ([]common.Result, error) {
 
 	return a.Results, nil
 }
+
+type PodInfo interface {
+	GetPodSpec() corev1.PodSpec
+}
+
+type DeploymentInfo struct {
+	*appsv1.Deployment
+}
+
+func (d DeploymentInfo) GetPodSpec() corev1.PodSpec {
+	return d.Spec.Template.Spec
+}
+
+// define a structure for ReplicationController
+type ReplicationControllerInfo struct {
+	*corev1.ReplicationController
+}
+
+func (rc ReplicationControllerInfo) GetPodSpec() corev1.PodSpec {
+	return rc.Spec.Template.Spec
+}
+
+// define a structure for ReplicaSet
+type ReplicaSetInfo struct {
+	*appsv1.ReplicaSet
+}
+
+func (rs ReplicaSetInfo) GetPodSpec() corev1.PodSpec {
+	return rs.Spec.Template.Spec
+}
+
+// define a structure for StatefulSet
+type StatefulSetInfo struct {
+	*appsv1.StatefulSet
+}
+
+// implement PodInfo for StatefulSetInfo
+func (ss StatefulSetInfo) GetPodSpec() corev1.PodSpec {
+	return ss.Spec.Template.Spec
+}

pkg/analyzer/hpaAnalyzer_test.go

@@ -10,6 +10,8 @@ import (
 	"github.com/magiconair/properties/assert"
 	appsv1 "k8s.io/api/apps/v1"
 	autoscalingv1 "k8s.io/api/autoscaling/v1"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/fake"
 )
@@ -163,7 +165,7 @@ func TestHPAAnalyzerWithNonExistentScaleTargetRef(t *testing.T) {
 	}
 }
 
-func TestHPAAnalyzerWithExistingScaleTargetRef(t *testing.T) {
+func TestHPAAnalyzerWithExistingScaleTargetRefAsDeployment(t *testing.T) {
 
 	clientset := fake.NewSimpleClientset(
 		&autoscalingv1.HorizontalPodAutoscaler{
@@ -185,6 +187,28 @@ func TestHPAAnalyzerWithExistingScaleTargetRef(t *testing.T) {
 				Namespace:   "default",
 				Annotations: map[string]string{},
 			},
+			Spec: appsv1.DeploymentSpec{
+				Template: corev1.PodTemplateSpec{
+					Spec: corev1.PodSpec{
+						Containers: []corev1.Container{
+							{
+								Name: "example",
+								Image: "nginx",
+								Resources: corev1.ResourceRequirements{
+									Requests: corev1.ResourceList{
+										"cpu": resource.MustParse("100m"),
+										"memory": resource.MustParse("128Mi"),
+									},
+									Limits: corev1.ResourceList{
+										"cpu": resource.MustParse("200m"),
+										"memory": resource.MustParse("256Mi"),
+									},
+								},
+							},
+						},
+					},
+				},
+			},
 		},
 	)
 	hpaAnalyzer := HpaAnalyzer{}
@@ -205,6 +229,265 @@ func TestHPAAnalyzerWithExistingScaleTargetRef(t *testing.T) {
 	}
 }
 
+func TestHPAAnalyzerWithExistingScaleTargetRefAsReplicationController(t *testing.T) {
+
+	clientset := fake.NewSimpleClientset(
+		&autoscalingv1.HorizontalPodAutoscaler{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:        "example",
+				Namespace:   "default",
+				Annotations: map[string]string{},
+			},
+			Spec: autoscalingv1.HorizontalPodAutoscalerSpec{
+				ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
+					Kind: "ReplicationController",
+					Name: "example",
+				},
+			},
+		},
+		&corev1.ReplicationController{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:        "example",
+				Namespace:   "default",
+				Annotations: map[string]string{},
+			},
+			Spec: corev1.ReplicationControllerSpec{
+				Template: &corev1.PodTemplateSpec{
+					Spec: corev1.PodSpec{
+						Containers: []corev1.Container{
+							{
+								Name: "example",
+								Image: "nginx",
+								Resources: corev1.ResourceRequirements{
+									Requests: corev1.ResourceList{
+										"cpu": resource.MustParse("100m"),
+										"memory": resource.MustParse("128Mi"),
+									},
+									Limits: corev1.ResourceList{
+										"cpu": resource.MustParse("200m"),
+										"memory": resource.MustParse("256Mi"),
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	)
+	hpaAnalyzer := HpaAnalyzer{}
+
+	config := common.Analyzer{
+		Client: &kubernetes.Client{
+			Client: clientset,
+		},
+		Context:   context.Background(),
+		Namespace: "default",
+	}
+	analysisResults, err := hpaAnalyzer.Analyze(config)
+	if err != nil {
+		t.Error(err)
+	}
+	for _, analysis := range analysisResults {
+		assert.Equal(t, len(analysis.Error), 0)
+	}
+}
+
+func TestHPAAnalyzerWithExistingScaleTargetRefAsReplicaSet(t *testing.T) {
+
+	clientset := fake.NewSimpleClientset(
+		&autoscalingv1.HorizontalPodAutoscaler{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:        "example",
+				Namespace:   "default",
+				Annotations: map[string]string{},
+			},
+			Spec: autoscalingv1.HorizontalPodAutoscalerSpec{
+				ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
+					Kind: "ReplicaSet",
+					Name: "example",
+				},
+			},
+		},
+		&appsv1.ReplicaSet{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:        "example",
+				Namespace:   "default",
+				Annotations: map[string]string{},
+			},
+			Spec: appsv1.ReplicaSetSpec{
+				Template: corev1.PodTemplateSpec{
+					Spec: corev1.PodSpec{
+						Containers: []corev1.Container{
+							{
+								Name: "example",
+								Image: "nginx",
+								Resources: corev1.ResourceRequirements{
+									Requests: corev1.ResourceList{
+										"cpu": resource.MustParse("100m"),
+										"memory": resource.MustParse("128Mi"),
+									},
+									Limits: corev1.ResourceList{
+										"cpu": resource.MustParse("200m"),
+										"memory": resource.MustParse("256Mi"),
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	)
+	hpaAnalyzer := HpaAnalyzer{}
+
+	config := common.Analyzer{
+		Client: &kubernetes.Client{
+			Client: clientset,
+		},
+		Context:   context.Background(),
+		Namespace: "default",
+	}
+	analysisResults, err := hpaAnalyzer.Analyze(config)
+	if err != nil {
+		t.Error(err)
+	}
+	for _, analysis := range analysisResults {
+		assert.Equal(t, len(analysis.Error), 0)
+	}
+}
+
+func TestHPAAnalyzerWithExistingScaleTargetRefAsStatefulSet(t *testing.T) {
+
+	clientset := fake.NewSimpleClientset(
+		&autoscalingv1.HorizontalPodAutoscaler{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:        "example",
+				Namespace:   "default",
+				Annotations: map[string]string{},
+			},
+			Spec: autoscalingv1.HorizontalPodAutoscalerSpec{
+				ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
+					Kind: "StatefulSet",
+					Name: "example",
+				},
+			},
+		},
+		&appsv1.StatefulSet{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:        "example",
+				Namespace:   "default",
+				Annotations: map[string]string{},
+			},
+			Spec: appsv1.StatefulSetSpec{
+				Template: corev1.PodTemplateSpec{
+					Spec: corev1.PodSpec{
+						Containers: []corev1.Container{
+							{
+								Name: "example",
+								Image: "nginx",
+								Resources: corev1.ResourceRequirements{
+									Requests: corev1.ResourceList{
+										"cpu": resource.MustParse("100m"),
+										"memory": resource.MustParse("128Mi"),
+									},
+									Limits: corev1.ResourceList{
+										"cpu": resource.MustParse("200m"),
+										"memory": resource.MustParse("256Mi"),
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	)
+	hpaAnalyzer := HpaAnalyzer{}
+
+	config := common.Analyzer{
+		Client: &kubernetes.Client{
+			Client: clientset,
+		},
+		Context:   context.Background(),
+		Namespace: "default",
+	}
+	analysisResults, err := hpaAnalyzer.Analyze(config)
+	if err != nil {
+		t.Error(err)
+	}
+	for _, analysis := range analysisResults {
+		assert.Equal(t, len(analysis.Error), 0)
+	}
+}
+
+func TestHPAAnalyzerWithExistingScaleTargetRefWithoutSpecifyingResources(t *testing.T) {
+
+	clientset := fake.NewSimpleClientset(
+		&autoscalingv1.HorizontalPodAutoscaler{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:        "example",
+				Namespace:   "default",
+				Annotations: map[string]string{},
+			},
+			Spec: autoscalingv1.HorizontalPodAutoscalerSpec{
+				ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
+					Kind: "Deployment",
+					Name: "example",
+				},
+			},
+		},
+		&appsv1.Deployment{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:        "example",
+				Namespace:   "default",
+				Annotations: map[string]string{},
+			},
+			Spec: appsv1.DeploymentSpec{
+				Template: corev1.PodTemplateSpec{
+					Spec: corev1.PodSpec{
+						Containers: []corev1.Container{
+							{
+								Name: "example",
+								Image: "nginx",
+							},
+						},
+					},
+				},
+			},
+		},
+	)
+	hpaAnalyzer := HpaAnalyzer{}
+
+	config := common.Analyzer{
+		Client: &kubernetes.Client{
+			Client: clientset,
+		},
+		Context:   context.Background(),
+		Namespace: "default",
+	}
+	analysisResults, err := hpaAnalyzer.Analyze(config)
+	if err != nil {
+		t.Error(err)
+	}
+
+	var errorFound bool
+	for _, analysis := range analysisResults {
+		for _, err := range analysis.Error {
+			if strings.Contains(err.Text, "does not have resource configured."){
+				errorFound = true
+				break
+			}
+			if errorFound {
+				break
+			}
+		}
+		if !errorFound {
+			t.Error("expected error 'does not have resource configured.' not found in analysis results")
+		}
+	}
+}
+
 func TestHPAAnalyzerNamespaceFiltering(t *testing.T) {
 	clientset := fake.NewSimpleClientset(
 		&autoscalingv1.HorizontalPodAutoscaler{