feat(prometheus client reconciling): be more strict about clearing the previous connection

To avoid stalling connections that are not expected to be kept. E.g.
when an invalid secret is provided.

Author: ingvagabund

pkg/descheduler/descheduler.go

@@ -289,6 +289,7 @@ func (d *inClusterPromClientController) reconcileInClusterSAToken() error {
 			klog.V(2).Infof("Creating Prometheus client (with SA token)")
 			prometheusClient, transport, err := d.createPrometheusClient(d.prometheusConfig.URL, cfg.BearerToken)
 			if err != nil {
+				d.clearConnection()
 				return fmt.Errorf("unable to create a prometheus client: %v", err)
 			}
 			d.promClient = prometheusClient
@@ -306,6 +307,23 @@ func (d *inClusterPromClientController) reconcileInClusterSAToken() error {
 	return fmt.Errorf("unexpected error when reading in cluster config: %v", err)
 }
 
+func clearPromClientConnection(currentPrometheusAuthToken *string, previousPrometheusClientTransport **http.Transport, promClient *promapi.Client) {
+	*currentPrometheusAuthToken = ""
+	if *previousPrometheusClientTransport != nil {
+		(*previousPrometheusClientTransport).CloseIdleConnections()
+	}
+	*previousPrometheusClientTransport = nil
+	*promClient = nil
+}
+
+func (d *inClusterPromClientController) clearConnection() {
+	clearPromClientConnection(&d.currentPrometheusAuthToken, &d.previousPrometheusClientTransport, &d.promClient)
+}
+
+func (d *secretBasedPromClientController) clearConnection() {
+	clearPromClientConnection(&d.currentPrometheusAuthToken, &d.previousPrometheusClientTransport, &d.promClient)
+}
+
 func (d *secretBasedPromClientController) runAuthenticationSecretReconciler(ctx context.Context) {
 	defer utilruntime.HandleCrash()
 	defer d.queue.ShutDown()
@@ -353,17 +371,13 @@ func (d *secretBasedPromClientController) sync() error {
 	if err != nil {
 		// clear the token if the secret is not found
 		if apierrors.IsNotFound(err) {
-			d.currentPrometheusAuthToken = ""
-			if d.previousPrometheusClientTransport != nil {
-				d.previousPrometheusClientTransport.CloseIdleConnections()
-			}
-			d.previousPrometheusClientTransport = nil
-			d.promClient = nil
+			d.clearConnection()
 		}
 		return fmt.Errorf("unable to get %v/%v secret", ns, name)
 	}
 	authToken := string(secretObj.Data[prometheusAuthTokenSecretKey])
 	if authToken == "" {
+		d.clearConnection()
 		return fmt.Errorf("prometheus authentication token secret missing %q data or empty", prometheusAuthTokenSecretKey)
 	}
 	if d.currentPrometheusAuthToken == authToken {
@@ -373,6 +387,7 @@ func (d *secretBasedPromClientController) sync() error {
 	klog.V(2).Infof("authentication secret token updated, recreating prometheus client")
 	prometheusClient, transport, err := d.createPrometheusClient(prometheusConfig.URL, authToken)
 	if err != nil {
+		d.clearConnection()
 		return fmt.Errorf("unable to create a prometheus client: %v", err)
 	}
 	d.promClient = prometheusClient

pkg/descheduler/descheduler_test.go

@@ -2175,14 +2175,15 @@ func TestPromClientControllerSync_ClientCreation(t *testing.T) {
 
 func TestPromClientControllerSync_EventHandler(t *testing.T) {
 	testCases := []struct {
-		name                             string
-		operation                        func(ctx context.Context, fakeClient *fakeclientset.Clientset) error
-		processItem                      bool
-		expectedPromClientSet            bool
-		expectedCreatedClientsCount      int
-		expectedCurrentToken             string
-		expectedPreviousTransportCleared bool
-		expectDifferentClients           bool
+		name                              string
+		operation                         func(ctx context.Context, fakeClient *fakeclientset.Clientset) error
+		processItem                       bool
+		expectedPromClientSet             bool
+		expectedCreatedClientsCount       int
+		expectedCurrentToken              string
+		expectedPreviousTransportCleared  bool
+		expectDifferentClients            bool
+		expectCreatePrometheusClientError bool
 	}{
 		// Check initial conditions
 		{
@@ -2219,15 +2220,56 @@ func TestPromClientControllerSync_EventHandler(t *testing.T) {
 			expectedCurrentToken:        "token-2",
 			expectDifferentClients:      true,
 		},
+		{
+			name: "update secret with invalid data",
+			operation: func(ctx context.Context, fakeClient *fakeclientset.Clientset) error {
+				secret := newPrometheusAuthSecret(withToken("token-3"))
+				secret.Data[prometheusAuthTokenSecretKey] = []byte{}
+				_, err := fakeClient.CoreV1().Secrets(secret.Namespace).Update(ctx, secret, metav1.UpdateOptions{})
+				return err
+			},
+			processItem:                 true,
+			expectedPromClientSet:       false,
+			expectedCreatedClientsCount: 2,
+			expectedCurrentToken:        "",
+			expectDifferentClients:      true,
+		},
+		{
+			name: "update secret with valid data",
+			operation: func(ctx context.Context, fakeClient *fakeclientset.Clientset) error {
+				secret := newPrometheusAuthSecret(withToken("token-4"))
+				_, err := fakeClient.CoreV1().Secrets(secret.Namespace).Update(ctx, secret, metav1.UpdateOptions{})
+				return err
+			},
+			processItem:                 true,
+			expectedPromClientSet:       true,
+			expectedCreatedClientsCount: 3,
+			expectedCurrentToken:        "token-4",
+			expectDifferentClients:      true,
+		},
+		{
+			name: "update secret with valid data but createPrometheusClient failing",
+			operation: func(ctx context.Context, fakeClient *fakeclientset.Clientset) error {
+				secret := newPrometheusAuthSecret(withToken("token-5"))
+				_, err := fakeClient.CoreV1().Secrets(secret.Namespace).Update(ctx, secret, metav1.UpdateOptions{})
+				return err
+			},
+			processItem:                       true,
+			expectedPromClientSet:             false,
+			expectedCreatedClientsCount:       3,
+			expectedCurrentToken:              "",
+			expectDifferentClients:            true,
+			expectCreatePrometheusClientError: true,
+		},
 		{
 			name: "delete secret",
 			operation: func(ctx context.Context, fakeClient *fakeclientset.Clientset) error {
-				secret := newPrometheusAuthSecret(withToken("token-2"))
+				secret := newPrometheusAuthSecret(withToken("token-5"))
 				return fakeClient.CoreV1().Secrets(secret.Namespace).Delete(ctx, secret.Name, metav1.DeleteOptions{})
 			},
 			processItem:                      true,
 			expectedPromClientSet:            false,
-			expectedCreatedClientsCount:      2,
+			expectedCreatedClientsCount:      3,
 			expectedCurrentToken:             "",
 			expectedPreviousTransportCleared: true,
 		},
@@ -2279,16 +2321,22 @@ func TestPromClientControllerSync_EventHandler(t *testing.T) {
 			// Track created clients to verify different instances
 			var createdClients []promapi.Client
 			var createdClientsMu sync.Mutex
-			ctrl.createPrometheusClient = func(url, token string) (promapi.Client, *http.Transport, error) {
-				client := &mockPrometheusClient{name: "client-" + token}
-				createdClientsMu.Lock()
-				createdClients = append(createdClients, client)
-				createdClientsMu.Unlock()
-				return client, &http.Transport{}, nil
-			}
 
 			for _, tc := range testCases {
 				t.Run(tc.name, func(t *testing.T) {
+					ctrl.mu.Lock()
+					ctrl.createPrometheusClient = func(url, token string) (promapi.Client, *http.Transport, error) {
+						if tc.expectCreatePrometheusClientError {
+							return nil, &http.Transport{}, fmt.Errorf("error creating a prometheus client")
+						}
+						client := &mockPrometheusClient{name: "client-" + token}
+						createdClientsMu.Lock()
+						createdClients = append(createdClients, client)
+						createdClientsMu.Unlock()
+						return client, &http.Transport{}, nil
+					}
+					ctrl.mu.Unlock()
+
 					if err := tc.operation(ctx, fakeClient); err != nil {
 						t.Fatalf("Failed to execute operation: %v", err)
 					}
@@ -2430,7 +2478,7 @@ func TestReconcileInClusterSAToken(t *testing.T) {
 			},
 			expectedErr:                    fmt.Errorf("unable to create a prometheus client: failed to create client"),
 			expectClientCreated:            false,
-			expectCurrentToken:             "old-token",
+			expectCurrentToken:             "",
 			expectPreviousTransportCleared: false,
 			expectPromClientCleared:        false,
 		},