CLOUDP-200130 Do not assume same order for members in Kubeconfig (#4110)

# Summary

Fixes a bug where we incorrectly assumed consistent ordering between
users and tokens in Kubeconfig.
[CLOUDP-200130](https://jira.mongodb.org/browse/CLOUDP-200130)

## Changes

- Fix: do not use `token := kubeConfig.Users[n].User.Token` to retrieve
user token. This is the change actually addressing the bug in the
ticket.
- Refactoring of `WatchMemberClusterHealth` to initialize the cache in a
separate function.
- Unit testing logic for extracting credentials from config.

## Proof of Work

New unit tests.

## Checklist
- [x] Have you linked a jira ticket and/or is the ticket in the title?
- [x] Have you checked whether your jira ticket required DOCSP changes?
- [x] Have you checked for release_note changes?

Author: Julien-Ben

RELEASE_NOTES.md

@@ -4,8 +4,7 @@
 
 ## Bug Fixes
 * Fixes the bug when status of `MongoDBUser` was being set to `Updated` prematurely. For example, new users were not immediately usable following `MongoDBUser` creation despite the operator reporting `Updated` state.
-
-<!-- Past releases -->
+* Fixed a bug causing cluster health check issues when ordering of users and tokens differed in Kubeconfig.
 
 # MongoDB Enterprise Kubernetes Operator 1.31.0
 

pkg/multicluster/memberwatch/memberwatch.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/json"
+	"fmt"
 	"math"
 	"time"
 
@@ -26,55 +27,75 @@ type MemberClusterHealthChecker struct {
 	Cache map[string]*MemberHeathCheck
 }
 
-// WatchMemberClusterHealth watches member clusters healthcheck. If a cluster fails healthcheck it re-enqueues the
-// MongoDBMultiCluster resources. It is spun up in the mongodb multi reconciler as a go-routine, and is executed every 10 seconds.
-func (m *MemberClusterHealthChecker) WatchMemberClusterHealth(ctx context.Context, log *zap.SugaredLogger, watchChannel chan event.GenericEvent, centralClient kubernetesClient.Client, clustersMap map[string]cluster.Cluster) {
-	// check if the local cache is populated if not let's do that
-	if len(m.Cache) == 0 {
-		// load the kubeconfig file contents from disk
-		kubeConfigFile, err := multicluster.NewKubeConfigFile(multicluster.GetKubeConfigPath())
-		if err != nil {
-			log.Errorf("Failed to read KubeConfig file err: %s", err)
-			// we can't populate the client so just bail out here
-			return
-		}
+type ClusterCredentials struct {
+	Server               string
+	CertificateAuthority []byte
+	Token                string
+}
 
-		kubeConfig, err := kubeConfigFile.LoadKubeConfigFile()
-		if err != nil {
-			log.Errorf("Failed to load the kubeconfig file content err: %s", err)
-			return
-		}
+func getClusterCredentials(clustersMap map[string]cluster.Cluster,
+	kubeConfig multicluster.KubeConfigFile,
+	kubeContext multicluster.KubeConfigContextItem,
+) (*ClusterCredentials, error) {
+	clusterName := kubeContext.Context.Cluster
+	if _, ok := clustersMap[clusterName]; !ok {
+		return nil, fmt.Errorf("cluster %s not found in clustersMap", clusterName)
+	}
 
-		for n := range kubeConfig.Contexts {
-			context := kubeConfig.Contexts[n]
-			clusterName := context.Context.Cluster
-			if _, ok := clustersMap[clusterName]; !ok {
-				continue
-			}
+	kubeCluster := getClusterFromContext(clusterName, kubeConfig.Clusters)
+	if kubeCluster == nil {
+		return nil, fmt.Errorf("failed to get cluster with clustername: %s, doesn't exists in Kubeconfig clusters", clusterName)
+	}
 
-			// fetch the cluster from the "clusters" with the given clusterName from the context
-			cluster := getClusterFromContext(clusterName, kubeConfig.Clusters)
-			if cluster == nil {
-				log.Errorf("Failed to get cluster with clustername: %s, doesn't exists in Kubeconfig clusters", clusterName)
-				continue
-			}
+	certificateAuthority, err := base64.StdEncoding.DecodeString(kubeCluster.CertificateAuthority)
+	if err != nil {
+		return nil, fmt.Errorf("failed to decode certificate for cluster: %s, err: %s", clusterName, err)
+	}
 
-			server := cluster.Server
-			certificateAuthority, err := base64.StdEncoding.DecodeString(cluster.CertificateAuthority)
-			if err != nil {
-				log.Errorf("Failed to decode certificate for cluster: %s, err: %s", clusterName, err)
-				continue
-			}
+	user := getUserFromContext(kubeContext.Context.User, kubeConfig.Users)
+	if user == nil {
+		return nil, fmt.Errorf("failed to get user with name: %s, doesn't exists in Kubeconfig users", kubeContext.Context.User)
+	}
 
-			// fetch the user from the "users" against the given user from the context
-			user := getUserFromContext(context.Context.User, kubeConfig.Users)
-			if user == nil {
-				log.Errorf("Failed to get user with clustername: %s, doesn't exists in Kubeconfig users", clusterName)
-				continue
-			}
-			token := kubeConfig.Users[n].User.Token
-			m.Cache[clusterName] = NewMemberHealthCheck(server, certificateAuthority, token, log)
+	return &ClusterCredentials{
+		Server:               kubeCluster.Server,
+		CertificateAuthority: certificateAuthority,
+		Token:                user.Token,
+	}, nil
+}
+
+func (m *MemberClusterHealthChecker) populateCache(clustersMap map[string]cluster.Cluster, log *zap.SugaredLogger) {
+	kubeConfigFile, err := multicluster.NewKubeConfigFile(multicluster.GetKubeConfigPath())
+	if err != nil {
+		log.Errorf("Failed to read KubeConfig file err: %s", err)
+		// we can't populate the client so just bail out here
+		return
+	}
+
+	kubeConfig, err := kubeConfigFile.LoadKubeConfigFile()
+	if err != nil {
+		log.Errorf("Failed to load the kubeconfig file content err: %s", err)
+		return
+	}
+
+	for n := range kubeConfig.Contexts {
+		kubeContext := kubeConfig.Contexts[n]
+		clusterName := kubeContext.Context.Cluster
+		credentials, err := getClusterCredentials(clustersMap, kubeConfig, kubeContext)
+		if err != nil {
+			log.Errorf("Skipping cluster %s: %v", clusterName, err)
+			continue
 		}
+		m.Cache[clusterName] = NewMemberHealthCheck(credentials.Server, credentials.CertificateAuthority, credentials.Token, log)
+	}
+}
+
+// WatchMemberClusterHealth watches member clusters healthcheck. If a cluster fails healthcheck it re-enqueues the
+// MongoDBMultiCluster resources. It is spun up in the mongodb multi reconciler as a go-routine, and is executed every 10 seconds.
+func (m *MemberClusterHealthChecker) WatchMemberClusterHealth(ctx context.Context, log *zap.SugaredLogger, watchChannel chan event.GenericEvent, centralClient kubernetesClient.Client, clustersMap map[string]cluster.Cluster) {
+	// check if the local cache is populated if not let's do that
+	if len(m.Cache) == 0 {
+		m.populateCache(clustersMap, log)
 	}
 
 	for {

pkg/multicluster/memberwatch/memberwatch_test.go

@@ -1,12 +1,16 @@
 package memberwatch
 
 import (
+	"encoding/base64"
 	"encoding/json"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sigs.k8s.io/controller-runtime/pkg/cluster"
 
 	"github.com/10gen/ops-manager-kubernetes/api/v1/mdb"
+	mc "github.com/10gen/ops-manager-kubernetes/pkg/multicluster"
 	"github.com/10gen/ops-manager-kubernetes/pkg/multicluster/failedcluster"
 )
 
@@ -147,3 +151,192 @@ func TestShouldAddFailedClusterAnnotation(t *testing.T) {
 		assert.Equal(t, shouldAddFailedClusterAnnotation(tt.annotations, tt.clusterName), tt.out)
 	}
 }
+
+func TestGetClusterCredentials(t *testing.T) {
+	validCertContent := "valid-cert"
+	validCert := base64.StdEncoding.EncodeToString([]byte(validCertContent))
+	invalidCert := "invalid-base64!!!"
+	clusterName := "cluster1"
+	userToken := "abc123"
+	mockUserItemList := []mc.KubeConfigUserItem{
+		{Name: "user1", User: mc.KubeConfigUser{Token: userToken}},
+	}
+	mockKubeContext := mc.KubeConfigContextItem{
+		Name: "context1",
+		Context: mc.KubeConfigContext{
+			Cluster: clusterName,
+			User:    "user1",
+		},
+	}
+	kubeconfigServerURL := "https://example.com"
+	mockKubeConfig := mc.KubeConfigFile{
+		Clusters: []mc.KubeConfigClusterItem{
+			{
+				Name: clusterName,
+				Cluster: mc.KubeConfigCluster{
+					Server:               kubeconfigServerURL,
+					CertificateAuthority: validCert,
+				},
+			},
+		},
+		Users: mockUserItemList,
+	}
+
+	tests := []struct {
+		name           string
+		clustersMap    map[string]cluster.Cluster // Using as a set; the value is not used.
+		kubeConfig     mc.KubeConfigFile
+		kubeContext    mc.KubeConfigContextItem
+		wantErr        bool
+		errContains    string
+		expectedServer string
+		expectedToken  string
+		expectedCA     []byte
+	}{
+		{
+			name:        "Cluster not in clustersMap",
+			clustersMap: map[string]cluster.Cluster{}, // Empty map; cluster1 is missing.
+			kubeConfig:  mockKubeConfig,
+			kubeContext: mockKubeContext,
+			wantErr:     true,
+			errContains: "cluster cluster1 not found in clustersMap",
+		},
+		{
+			name: "Cluster missing in kubeConfig.Clusters",
+			clustersMap: map[string]cluster.Cluster{
+				clusterName: nil,
+			},
+			kubeConfig: mc.KubeConfigFile{
+				Clusters: []mc.KubeConfigClusterItem{}, // No cluster defined.
+				Users:    mockUserItemList,
+			},
+			kubeContext: mockKubeContext,
+			wantErr:     true,
+			errContains: "failed to get cluster with clustername: cluster1",
+		},
+		{
+			name: "Invalid certificate authority",
+			clustersMap: map[string]cluster.Cluster{
+				clusterName: nil,
+			},
+			kubeConfig: mc.KubeConfigFile{
+				Clusters: []mc.KubeConfigClusterItem{
+					{
+						Name: clusterName,
+						Cluster: mc.KubeConfigCluster{
+							Server:               kubeconfigServerURL,
+							CertificateAuthority: invalidCert, // The kubeConfig has an invalid CA
+						},
+					},
+				},
+				Users: mockUserItemList,
+			},
+			kubeContext: mockKubeContext,
+			wantErr:     true,
+			errContains: "failed to decode certificate for cluster: cluster1",
+		},
+		{
+			name: "User not found",
+			clustersMap: map[string]cluster.Cluster{
+				clusterName: nil,
+			},
+			kubeConfig: mc.KubeConfigFile{
+				Clusters: []mc.KubeConfigClusterItem{
+					{
+						Name: clusterName,
+						Cluster: mc.KubeConfigCluster{
+							Server:               kubeconfigServerURL,
+							CertificateAuthority: validCert,
+						},
+					},
+				},
+				Users: []mc.KubeConfigUserItem{}, // No users defined.
+			},
+			kubeContext: mc.KubeConfigContextItem{
+				Name: "context1",
+				Context: mc.KubeConfigContext{
+					Cluster: clusterName,
+					User:    "user1", // User is not present.
+				},
+			},
+			wantErr:     true,
+			errContains: "failed to get user with name: user1",
+		},
+		{
+			name: "Successful extraction",
+			clustersMap: map[string]cluster.Cluster{
+				clusterName: nil,
+			},
+			kubeConfig:     mockKubeConfig,
+			kubeContext:    mockKubeContext,
+			wantErr:        false,
+			expectedServer: kubeconfigServerURL,
+			expectedToken:  userToken,
+			expectedCA:     []byte(validCertContent),
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			creds, err := getClusterCredentials(tc.clustersMap, tc.kubeConfig, tc.kubeContext)
+			if tc.wantErr {
+				assert.ErrorContains(t, err, tc.errContains)
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, tc.expectedServer, creds.Server)
+				assert.Equal(t, tc.expectedToken, creds.Token)
+				assert.Equal(t, tc.expectedCA, creds.CertificateAuthority)
+			}
+		})
+	}
+}
+
+func TestGetUserFromContext(t *testing.T) {
+	tests := []struct {
+		name         string
+		userName     string
+		users        []mc.KubeConfigUserItem
+		expectedUser *mc.KubeConfigUser
+	}{
+		{
+			name:     "User exists",
+			userName: "alice",
+			users: []mc.KubeConfigUserItem{
+				{Name: "alice", User: mc.KubeConfigUser{Token: "alice-token"}},
+				{Name: "bob", User: mc.KubeConfigUser{Token: "bob-token"}},
+			},
+			expectedUser: &mc.KubeConfigUser{Token: "alice-token"},
+		},
+		{
+			name:     "User does not exist",
+			userName: "charlie",
+			users: []mc.KubeConfigUserItem{
+				{Name: "alice", User: mc.KubeConfigUser{Token: "alice-token"}},
+				{Name: "bob", User: mc.KubeConfigUser{Token: "bob-token"}},
+			},
+			expectedUser: nil,
+		},
+		{
+			name:         "Empty users slice",
+			userName:     "alice",
+			users:        []mc.KubeConfigUserItem{},
+			expectedUser: nil,
+		},
+		{
+			name:     "Multiple users with same name, returns first match",
+			userName: "duplicated",
+			users: []mc.KubeConfigUserItem{
+				{Name: "duplicated", User: mc.KubeConfigUser{Token: "first-token"}},
+				{Name: "duplicated", User: mc.KubeConfigUser{Token: "second-token"}},
+			},
+			expectedUser: &mc.KubeConfigUser{Token: "first-token"},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			user := getUserFromContext(tc.userName, tc.users)
+			assert.Equal(t, tc.expectedUser, user)
+		})
+	}
+}