OCPBUGS-8059: Stop Clusterversion caching (openshift#752)

* Stop Clusterversion caching

* Unit test dev

* Unit test dev, insights client struct and constructor changes

* unit test refactoring

* depricated package fix in unit test

* update based on review

* update based on review

* use client created from gatherKubeConfig

* minor name refactor

pkg/controller/gather_job.go

@@ -85,14 +85,22 @@ func (d *GatherJob) Gather(ctx context.Context, kubeConfig, protoKubeConfig *res
 	recdriver := diskrecorder.New(d.StoragePath)
 	rec := recorder.New(recdriver, d.Interval, anonymizer)
 	defer func() {
-		if err := rec.Flush(); err != nil {
+		if err = rec.Flush(); err != nil {
 			klog.Error(err)
 		}
 	}()
 
 	authorizer := clusterauthorizer.New(configObserver)
-	insightsClient := insightsclient.New(nil, 0, "default", authorizer, gatherKubeConfig)
 
+	// gatherConfigClient is configClient created from gatherKubeConfig, this name was used because configClient was already taken
+	// this client is only used in insightsClient, it is created here
+	// because pkg/insights/insightsclient/request_test.go unit test won't work otherwise
+	gatherConfigClient, err := configv1client.NewForConfig(gatherKubeConfig)
+	if err != nil {
+		return err
+	}
+
+	insightsClient := insightsclient.New(nil, 0, "default", authorizer, gatherConfigClient)
 	gatherers := gather.CreateAllGatherers(
 		gatherKubeConfig, gatherProtoKubeConfig, metricsGatherKubeConfig, alertsGatherKubeConfig, anonymizer,
 		configObserver, insightsClient,

pkg/controller/operator.go

@@ -127,7 +127,16 @@ func (s *Operator) Run(ctx context.Context, controller *controllercmd.Controller
 	go rec.PeriodicallyPrune(ctx, statusReporter)
 
 	authorizer := clusterauthorizer.New(secretConfigObserver)
-	insightsClient := insightsclient.New(nil, 0, "default", authorizer, gatherKubeConfig)
+
+	// gatherConfigClient is configClient created from gatherKubeConfig, this name was used because configClient was already taken
+	// this client is only used in insightsClient, it is created here
+	// because pkg/insights/insightsclient/request_test.go unit test won't work otherwise
+	gatherConfigClient, err := configv1client.NewForConfig(gatherKubeConfig)
+	if err != nil {
+		return err
+	}
+
+	insightsClient := insightsclient.New(nil, 0, "default", authorizer, gatherConfigClient)
 
 	// the gatherers are periodically called to collect the data from the cluster
 	// and provide the results for the recorder

pkg/insights/insightsclient/insightsclient.go

@@ -18,14 +18,13 @@ import (
 	"time"
 
 	"k8s.io/client-go/pkg/version"
-	"k8s.io/client-go/rest"
 	"k8s.io/client-go/transport"
 	"k8s.io/component-base/metrics"
 
 	"k8s.io/klog/v2"
 
 	configv1 "github.com/openshift/api/config/v1"
-	configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
+	configv1client "github.com/openshift/client-go/config/clientset/versioned"
 	"github.com/openshift/insights-operator/pkg/insights"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	apimachineryversion "k8s.io/apimachinery/pkg/version"
@@ -38,13 +37,11 @@ const (
 )
 
 type Client struct {
-	client      *http.Client
-	maxBytes    int64
-	metricsName string
-
-	authorizer       Authorizer
-	gatherKubeConfig *rest.Config
-	clusterVersion   *configv1.ClusterVersion
+	client       *http.Client
+	maxBytes     int64
+	metricsName  string
+	authorizer   Authorizer
+	configClient *configv1client.Clientset
 }
 
 type Authorizer interface {
@@ -92,19 +89,19 @@ func IsHttpError(err error) bool {
 var ErrWaitingForVersion = fmt.Errorf("waiting for the cluster version to be loaded")
 
 // New creates a Client
-func New(client *http.Client, maxBytes int64, metricsName string, authorizer Authorizer, gatherKubeConfig *rest.Config) *Client {
+func New(client *http.Client, maxBytes int64, metricsName string, authorizer Authorizer, configClient *configv1client.Clientset) *Client {
 	if client == nil {
 		client = &http.Client{}
 	}
 	if maxBytes == 0 {
 		maxBytes = 10 * 1024 * 1024
 	}
 	return &Client{
-		client:           client,
-		maxBytes:         maxBytes,
-		metricsName:      metricsName,
-		authorizer:       authorizer,
-		gatherKubeConfig: gatherKubeConfig,
+		client:       client,
+		maxBytes:     maxBytes,
+		metricsName:  metricsName,
+		authorizer:   authorizer,
+		configClient: configClient,
 	}
 }
 
@@ -162,22 +159,13 @@ func userAgent(releaseVersionEnv string, v apimachineryversion.Info, cv *configv
 }
 
 func (c *Client) GetClusterVersion() (*configv1.ClusterVersion, error) {
-	if c.clusterVersion != nil {
-		return c.clusterVersion, nil
-	}
 	ctx := context.Background()
 
-	gatherConfigClient, err := configv1client.NewForConfig(c.gatherKubeConfig)
-	if err != nil {
-		return nil, err
-	}
-
-	cv, err := gatherConfigClient.ClusterVersions().Get(ctx, "version", metav1.GetOptions{})
+	cv, err := c.configClient.ConfigV1().ClusterVersions().Get(ctx, "version", metav1.GetOptions{})
 	if err != nil {
 		return nil, err
 	}
 
-	c.clusterVersion = cv
 	return cv, nil
 }
 

pkg/insights/insightsclient/requests_test.go

@@ -1,15 +1,21 @@
 package insightsclient
 
 import (
+	"bytes"
 	"context"
+	"encoding/json"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"testing"
 
 	configv1 "github.com/openshift/api/config/v1"
+	configv1client "github.com/openshift/client-go/config/clientset/versioned"
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/scheme"
+	fakerest "k8s.io/client-go/rest/fake"
 )
 
 const testRules = `{
@@ -38,19 +44,15 @@ const testRules = `{
 }`
 
 func TestClient_RecvGatheringRules(t *testing.T) {
-	httpClient := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
+	httpServer := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
 		writer.WriteHeader(http.StatusOK)
 		_, err := writer.Write([]byte(testRules))
-		if err != nil {
-			assert.NoError(t, err)
-		}
+		assert.NoError(t, err)
 	}))
-	defer httpClient.Close()
-
-	endpoint := httpClient.URL
+	endpoint := httpServer.URL
+	defer httpServer.Close()
 
-	client := New(http.DefaultClient, 0, "", &MockAuthorizer{}, nil)
-	client.clusterVersion = &configv1.ClusterVersion{
+	clusterVersion := &configv1.ClusterVersion{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "version",
 		},
@@ -59,11 +61,25 @@ func TestClient_RecvGatheringRules(t *testing.T) {
 			Channel:   "stable-4.9",
 		},
 	}
-	gatheringRulesBytes, err := client.RecvGatheringRules(context.TODO(), endpoint)
-	if err != nil {
-		assert.NoError(t, err)
+	cv, err := json.Marshal(clusterVersion)
+	assert.NoError(t, err)
+
+	fakeClient := &fakerest.RESTClient{
+		Client: fakerest.CreateHTTPClient(func(request *http.Request) (*http.Response, error) {
+			resp := &http.Response{
+				StatusCode: http.StatusOK,
+				Body:       io.NopCloser(bytes.NewReader(cv)),
+			}
+			return resp, nil
+		}),
+		NegotiatedSerializer: scheme.Codecs.WithoutConversion(),
+		GroupVersion:         configv1.GroupVersion,
 	}
 
+	configClient := configv1client.New(fakeClient)
+	insightsClient := New(http.DefaultClient, 0, "", &MockAuthorizer{}, configClient)
+	gatheringRulesBytes, err := insightsClient.RecvGatheringRules(context.Background(), endpoint)
+	assert.NoError(t, err)
 	assert.JSONEq(t, testRules, string(gatheringRulesBytes))
 }