SREP-701: refactor getClusterVersion into util package

Author: rolandmkunkel

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

@@ -1,7 +1,6 @@
 package cannotretrieveupdatessre
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"strings"
@@ -12,7 +11,8 @@ import (
 	"github.com/openshift/configuration-anomaly-detection/pkg/logging"
 	"github.com/openshift/configuration-anomaly-detection/pkg/networkverifier"
 	"github.com/openshift/configuration-anomaly-detection/pkg/notewriter"
-	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/openshift/configuration-anomaly-detection/pkg/investigations/utils/version"
 )
 
 const (
@@ -53,7 +53,7 @@ func (c *Investigation) Run(r *investigation.Resources) (investigation.Investiga
 	}
 
 	// Check ClusterVersion
-	clusterVersion, err := getClusterVersion(k8scli)
+	clusterVersion, err := version.GetClusterVersion(k8scli)
 	if err != nil {
 		notes.AppendWarning("Failed to get ClusterVersion: %s", err.Error())
 	} else {
@@ -70,15 +70,6 @@ func (c *Investigation) Run(r *investigation.Resources) (investigation.Investiga
 	return result, r.PdClient.EscalateIncidentWithNote(notes.String())
 }
 
-func getClusterVersion(k8scli client.Client) (*configv1.ClusterVersion, error) {
-	clusterVersion := &configv1.ClusterVersion{}
-	err := k8scli.Get(context.TODO(), client.ObjectKey{Name: "version"}, clusterVersion)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get ClusterVersion: %w", err)
-	}
-	return clusterVersion, nil
-}
-
 // getUpdateRetrievalFailures checks for update retrieval failures in the ClusterVersion
 func getUpdateRetrievalFailures(clusterVersion *configv1.ClusterVersion) string {
 	for _, condition := range clusterVersion.Status.Conditions {

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre_test.go

@@ -1,97 +1,12 @@
 package cannotretrieveupdatessre
 
 import (
-	"strings"
 	"testing"
 
 	configv1 "github.com/openshift/api/config/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes/scheme"
-	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
-func newFakeClient(objs ...client.Object) (client.Client, error) {
-	s := scheme.Scheme
-	err := configv1.AddToScheme(s)
-	if err != nil {
-		return nil, err
-	}
-
-	client := fake.NewClientBuilder().WithScheme(s).WithObjects(objs...).Build()
-	return client, nil
-}
-
-func TestGetClusterVersion(t *testing.T) {
-	tests := []struct {
-		name            string
-		clusterVersion  *configv1.ClusterVersion
-		expectedVersion string
-		expectError     bool
-	}{
-		{
-			name: "Valid ClusterVersion",
-			clusterVersion: &configv1.ClusterVersion{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "version",
-				},
-				Spec: configv1.ClusterVersionSpec{
-					Channel:   "stable-4.18",
-					ClusterID: "d1ba89f3-fd3e-48d2-91c6-test",
-				},
-				Status: configv1.ClusterVersionStatus{
-					Desired: configv1.Release{Version: "4.18.10"},
-				},
-			},
-			expectedVersion: "4.18.10",
-			expectError:     false,
-		},
-		{
-			name:            "ClusterVersion Not Found",
-			clusterVersion:  nil,
-			expectedVersion: "",
-			expectError:     true,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			var k8scli client.Client
-			var err error
-			if tt.clusterVersion != nil {
-				k8scli, err = newFakeClient(tt.clusterVersion)
-			} else {
-				k8scli, err = newFakeClient()
-			}
-			if err != nil {
-				t.Fatalf("failed to create a fake client: %v", err)
-			}
-
-			got, err := getClusterVersion(k8scli)
-
-			if tt.expectError && err == nil {
-				t.Errorf("Expected an error, got none")
-			} else if !tt.expectError && err != nil {
-				t.Errorf("Expected no error, got %v", err)
-			}
-
-			if !tt.expectError {
-				if got.Status.Desired.Version != tt.expectedVersion {
-					t.Errorf("Expected version %q, got %q", tt.expectedVersion, got.Status.Desired.Version)
-				}
-			} else {
-				if got != nil {
-					t.Errorf("Expected nil ClusterVersion error, got %v", got)
-				}
-				if err != nil && !apierrors.IsNotFound(err) && !strings.Contains(err.Error(), "failed to get ClusterVersion") {
-					t.Errorf("Expected error to be related about failed to get the ClusterVersion, got %v", err)
-				}
-			}
-		})
-	}
-}
-
 func TestGetUpdateRetrievalFailures(t *testing.T) {
 	tests := []struct {
 		name           string

pkg/investigations/machinehealthcheckunterminatedshortcircuitsre/machinehealthcheckunterminatedshortcircuitsre.go

@@ -29,8 +29,8 @@ const (
 )
 
 type Investigation struct {
-	// kclient provides access to on-cluster resources
-	kclient k8sclient.Client
+	// k8scli provides access to on-cluster resources
+	k8scli k8sclient.Client
 	// notes holds the messages that will be shared with Primary upon completion
 	notes *notewriter.NoteWriter
 	// recommendations holds the set of actions CAD recommends primary to take
@@ -39,19 +39,19 @@ type Investigation struct {
 
 func (i *Investigation) setup(r *investigation.Resources) error {
 	// Setup investigation
-	k, err := k8sclient.New(r.Cluster.ID(), r.OcmClient, r.Name)
+	k8scli, err := k8sclient.New(r.Cluster.ID(), r.OcmClient, r.Name)
 	if err != nil {
 		return fmt.Errorf("failed to initialize kubernetes client: %w", err)
 	}
-	i.kclient = k
+	i.k8scli = k8scli
 	i.notes = notewriter.New(r.Name, logging.RawLogger)
 	i.recommendations = investigationRecommendations{}
 
 	return nil
 }
 
 func (i *Investigation) teardown() error {
-	return i.kclient.Clean()
+	return i.k8scli.Clean()
 }
 
 // Run investigates the MachineHealthCheckUnterminatedShortCircuitSRE alert
@@ -106,7 +106,7 @@ func (i *Investigation) Run(r *investigation.Resources) (investigation.Investiga
 	// If one or more machines managed by the machinehealthcheck have not yet been identified as a problem, check on the machine's
 	// node to determine if there are node-level problems that need remediating
 	if len(targetMachines) > 0 {
-		targetNodes, err := machineutil.GetNodesForMachines(ctx, i.kclient, targetMachines)
+		targetNodes, err := machineutil.GetNodesForMachines(ctx, i.k8scli, targetMachines)
 		if err != nil {
 			i.notes.AppendWarning("failed to retrieve one or more target nodes: %v", err)
 		}
@@ -127,15 +127,15 @@ func (i *Investigation) Run(r *investigation.Resources) (investigation.Investiga
 
 func (i *Investigation) getMachinesFromFailingMHC(ctx context.Context) ([]machinev1beta1.Machine, error) {
 	healthchecks := machinev1beta1.MachineHealthCheckList{}
-	err := i.kclient.List(ctx, &healthchecks, &client.ListOptions{Namespace: machineutil.MachineNamespace})
+	err := i.k8scli.List(ctx, &healthchecks, &client.ListOptions{Namespace: machineutil.MachineNamespace})
 	if err != nil {
 		return []machinev1beta1.Machine{}, fmt.Errorf("failed to retrieve machinehealthchecks from cluster: %w", err)
 	}
 
 	targets := []machinev1beta1.Machine{}
 	for _, healthcheck := range healthchecks.Items {
 		if !machineutil.HealthcheckRemediationAllowed(healthcheck) {
-			machines, err := machineutil.GetMachinesForMHC(ctx, i.kclient, healthcheck)
+			machines, err := machineutil.GetMachinesForMHC(ctx, i.k8scli, healthcheck)
 			if err != nil {
 				i.notes.AppendWarning("failed to retrieve machines from machinehealthcheck %q: %v", healthcheck.Name, err)
 				continue
@@ -240,7 +240,7 @@ func (i *Investigation) investigateDeletingMachine(ctx context.Context, machine
 		i.recommendations.addRecommendation(recommendationInvestigateMachine, machine.Name, notes)
 		return nil
 	}
-	node, err := machineutil.GetNodeForMachine(ctx, i.kclient, machine)
+	node, err := machineutil.GetNodeForMachine(ctx, i.k8scli, machine)
 	if err != nil {
 		notes := "machine's node could not be determined"
 		i.recommendations.addRecommendation(recommendationInvestigateMachine, machine.Name, notes)

pkg/investigations/machinehealthcheckunterminatedshortcircuitsre/machinehealthcheckunterminatedshortcircuitsre_test.go

@@ -573,7 +573,7 @@ func newTestInvestigation(testObjects ...client.Object) (Investigation, error) {
 	}
 
 	i := Investigation{
-		kclient:         clientImpl{fakeClient},
+		k8scli:          clientImpl{fakeClient},
 		notes:           notewriter.New("testing", logging.RawLogger),
 		recommendations: investigationRecommendations{},
 	}

pkg/investigations/utils/machine/machine.go

@@ -30,13 +30,13 @@ func HealthcheckRemediationAllowed(healthcheck machinev1beta1.MachineHealthCheck
 }
 
 // GetMachinesForMHC retrieves the machines managed by the given MachineHealthCheck object
-func GetMachinesForMHC(ctx context.Context, kclient client.Client, healthcheck machinev1beta1.MachineHealthCheck) ([]machinev1beta1.Machine, error) {
+func GetMachinesForMHC(ctx context.Context, k8scli client.Client, healthcheck machinev1beta1.MachineHealthCheck) ([]machinev1beta1.Machine, error) {
 	machines := machinev1beta1.MachineList{}
 	selector, err := metav1.LabelSelectorAsSelector(&healthcheck.Spec.Selector)
 	if err != nil {
 		return []machinev1beta1.Machine{}, fmt.Errorf("failed to convert machinehealthcheck %q .spec.selector: %w", healthcheck.Name, err)
 	}
-	err = kclient.List(ctx, &machines, client.MatchingLabelsSelector{Selector: selector}, &client.ListOptions{Namespace: MachineNamespace})
+	err = k8scli.List(ctx, &machines, client.MatchingLabelsSelector{Selector: selector}, &client.ListOptions{Namespace: MachineNamespace})
 	if err != nil {
 		return []machinev1beta1.Machine{}, fmt.Errorf("failed to retrieve machines from machinehealthcheck %q: %w", healthcheck.Name, err)
 	}
@@ -53,9 +53,9 @@ func GetRole(machine machinev1beta1.Machine) (string, error) {
 }
 
 // GetNodesForMachines retrieves the nodes for the given machines. Errors encountered are joined, but do not block the retrieval of other machines
-func GetNodesForMachines(ctx context.Context, kclient client.Client, machines []machinev1beta1.Machine) ([]corev1.Node, error) {
+func GetNodesForMachines(ctx context.Context, k8scli client.Client, machines []machinev1beta1.Machine) ([]corev1.Node, error) {
 	// Retrieving all nodes initially & filtering out irrelevant objects results in fewer API calls
-	nodes, err := node.GetAll(ctx, kclient)
+	nodes, err := node.GetAll(ctx, k8scli)
 	if err != nil {
 		return []corev1.Node{}, fmt.Errorf("failed to retrieve nodes: %w", err)
 	}
@@ -87,11 +87,11 @@ func findMatchingNode(machine machinev1beta1.Machine, nodes []corev1.Node) (core
 
 // GetNodeForMachine retrieves the node for the given machine. If the provided machine's .Status.NodeRef is empty,
 // an error is returned
-func GetNodeForMachine(ctx context.Context, kclient client.Client, machine machinev1beta1.Machine) (corev1.Node, error) {
+func GetNodeForMachine(ctx context.Context, k8scli client.Client, machine machinev1beta1.Machine) (corev1.Node, error) {
 	if machine.Status.NodeRef == nil || machine.Status.NodeRef.Name == "" {
 		return corev1.Node{}, fmt.Errorf("no .Status.NodeRef defined for machine %q", machine.Name)
 	}
 	node := &corev1.Node{}
-	err := kclient.Get(ctx, types.NamespacedName{Name: machine.Status.NodeRef.Name}, node)
+	err := k8scli.Get(ctx, types.NamespacedName{Name: machine.Status.NodeRef.Name}, node)
 	return *node, err
 }

pkg/investigations/utils/machine/machine_test.go

@@ -264,13 +264,13 @@ func Test_GetNodeForMachine(t *testing.T) {
 				objs = append(objs, node)
 			}
 
-			kclient, err := newFakeClient(objs...)
+			k8scli, err := newFakeClient(objs...)
 			if err != nil {
 				t.Errorf("failed to create fake client for testing: %v", err)
 			}
 
 			// Execute
-			got, err := GetNodeForMachine(context.TODO(), kclient, *machine)
+			got, err := GetNodeForMachine(context.TODO(), k8scli, *machine)
 
 			// Validate results
 			if (err != nil) != tt.want.err {

pkg/investigations/utils/node/node.go

@@ -29,9 +29,9 @@ func FindNoScheduleTaint(node corev1.Node) (corev1.Taint, bool) {
 }
 
 // GetNodes retrieves all nodes present in the cluster
-func GetAll(ctx context.Context, kclient client.Client) ([]corev1.Node, error) {
+func GetAll(ctx context.Context, k8scli client.Client) ([]corev1.Node, error) {
 	nodes := corev1.NodeList{}
-	err := kclient.List(ctx, &nodes)
+	err := k8scli.List(ctx, &nodes)
 	return nodes.Items, err
 }
 

pkg/investigations/utils/version/version.go

@@ -0,0 +1,18 @@
+package version
+
+import (
+	"context"
+	"fmt"
+
+	configv1 "github.com/openshift/api/config/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func GetClusterVersion(k8scli client.Client) (*configv1.ClusterVersion, error) {
+	clusterVersion := &configv1.ClusterVersion{}
+	err := k8scli.Get(context.TODO(), client.ObjectKey{Name: "version"}, clusterVersion)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get ClusterVersion: %w", err)
+	}
+	return clusterVersion, nil
+}