Merge pull request #90836 from Danil-Grigorev/update-connection-on-secret-change

Added ability for vSphere to reconnect on secret update

staging/src/k8s.io/legacy-cloud-providers/vsphere/BUILD

@@ -79,6 +79,7 @@ go_test(
         "//vendor/github.com/vmware/govmomi/vapi/tags:go_default_library",
         "//vendor/github.com/vmware/govmomi/vim25/mo:go_default_library",
         "//vendor/github.com/vmware/govmomi/vim25/types:go_default_library",
+        "//vendor/k8s.io/klog/v2:go_default_library",
     ],
 )
 

staging/src/k8s.io/legacy-cloud-providers/vsphere/nodemanager.go

@@ -281,34 +281,7 @@ func (nm *NodeManager) removeNode(node *v1.Node) {
 //
 // This method is a getter but it can cause side-effect of updating NodeInfo object.
 func (nm *NodeManager) GetNodeInfo(nodeName k8stypes.NodeName) (NodeInfo, error) {
-	getNodeInfo := func(nodeName k8stypes.NodeName) *NodeInfo {
+	return nm.getRefreshedNodeInfo(nodeName)
-		nm.nodeInfoLock.RLock()
-		nodeInfo := nm.nodeInfoMap[convertToString(nodeName)]
-		nm.nodeInfoLock.RUnlock()
-		return nodeInfo
-	}
-	nodeInfo := getNodeInfo(nodeName)
-	var err error
-	if nodeInfo == nil {
-		// Rediscover node if no NodeInfo found.
-		klog.V(4).Infof("No VM found for node %q. Initiating rediscovery.", convertToString(nodeName))
-		err = nm.RediscoverNode(nodeName)
-		if err != nil {
-			klog.Errorf("Error %q node info for node %q not found", err, convertToString(nodeName))
-			return NodeInfo{}, err
-		}
-		nodeInfo = getNodeInfo(nodeName)
-	} else {
-		// Renew the found NodeInfo to avoid stale vSphere connection.
-		klog.V(4).Infof("Renewing NodeInfo %+v for node %q", nodeInfo, convertToString(nodeName))
-		nodeInfo, err = nm.renewNodeInfo(nodeInfo, true)
-		if err != nil {
-			klog.Errorf("Error %q occurred while renewing NodeInfo for %q", err, convertToString(nodeName))
-			return NodeInfo{}, err
-		}
-		nm.addNodeInfo(convertToString(nodeName), nodeInfo)
-	}
-	return *nodeInfo, nil
 }
 
 // GetNodeDetails returns NodeDetails for all the discovered nodes.
@@ -330,12 +303,59 @@ func (nm *NodeManager) GetNodeDetails() ([]NodeDetails, error) {
 	return nodeDetails, nil
 }
 
+func (nm *NodeManager) refreshNodes() (errList []error) {
+	nm.registeredNodesLock.Lock()
+	defer nm.registeredNodesLock.Unlock()
+
+	for nodeName := range nm.registeredNodes {
+		nodeInfo, err := nm.getRefreshedNodeInfo(convertToK8sType(nodeName))
+		if err != nil {
+			errList = append(errList, err)
+			continue
+		}
+		klog.V(4).Infof("Updated NodeInfo %v for node %q.", nodeInfo, nodeName)
+	}
+	return errList
+}
+
+func (nm *NodeManager) getRefreshedNodeInfo(nodeName k8stypes.NodeName) (NodeInfo, error) {
+	nodeInfo := nm.getNodeInfo(nodeName)
+	var err error
+	if nodeInfo == nil {
+		// Rediscover node if no NodeInfo found.
+		klog.V(4).Infof("No VM found for node %q. Initiating rediscovery.", convertToString(nodeName))
+		err = nm.RediscoverNode(nodeName)
+		if err != nil {
+			klog.Errorf("Error %q node info for node %q not found", err, convertToString(nodeName))
+			return NodeInfo{}, err
+		}
+		nodeInfo = nm.getNodeInfo(nodeName)
+	} else {
+		// Renew the found NodeInfo to avoid stale vSphere connection.
+		klog.V(4).Infof("Renewing NodeInfo %+v for node %q", nodeInfo, convertToString(nodeName))
+		nodeInfo, err = nm.renewNodeInfo(nodeInfo, true)
+		if err != nil {
+			klog.Errorf("Error %q occurred while renewing NodeInfo for %q", err, convertToString(nodeName))
+			return NodeInfo{}, err
+		}
+		nm.addNodeInfo(convertToString(nodeName), nodeInfo)
+	}
+	return *nodeInfo, nil
+}
+
 func (nm *NodeManager) addNodeInfo(nodeName string, nodeInfo *NodeInfo) {
 	nm.nodeInfoLock.Lock()
 	nm.nodeInfoMap[nodeName] = nodeInfo
 	nm.nodeInfoLock.Unlock()
 }
 
+func (nm *NodeManager) getNodeInfo(nodeName k8stypes.NodeName) *NodeInfo {
+	nm.nodeInfoLock.RLock()
+	nodeInfo := nm.nodeInfoMap[convertToString(nodeName)]
+	nm.nodeInfoLock.RUnlock()
+	return nodeInfo
+}
+
 func (nm *NodeManager) GetVSphereInstance(nodeName k8stypes.NodeName) (VSphereInstance, error) {
 	nodeInfo, err := nm.GetNodeInfo(nodeName)
 	if err != nil {
@@ -417,35 +437,7 @@ func (nm *NodeManager) vcConnect(ctx context.Context, vsphereInstance *VSphereIn
 //
 // This method is a getter but it can cause side-effect of updating NodeInfo object.
 func (nm *NodeManager) GetNodeInfoWithNodeObject(node *v1.Node) (NodeInfo, error) {
-	nodeName := node.Name
+	return nm.getRefreshedNodeInfo(convertToK8sType(node.Name))
-	getNodeInfo := func(nodeName string) *NodeInfo {
-		nm.nodeInfoLock.RLock()
-		nodeInfo := nm.nodeInfoMap[nodeName]
-		nm.nodeInfoLock.RUnlock()
-		return nodeInfo
-	}
-	nodeInfo := getNodeInfo(nodeName)
-	var err error
-	if nodeInfo == nil {
-		// Rediscover node if no NodeInfo found.
-		klog.V(4).Infof("No VM found for node %q. Initiating rediscovery.", nodeName)
-		err = nm.DiscoverNode(node)
-		if err != nil {
-			klog.Errorf("Error %q node info for node %q not found", err, nodeName)
-			return NodeInfo{}, err
-		}
-		nodeInfo = getNodeInfo(nodeName)
-	} else {
-		// Renew the found NodeInfo to avoid stale vSphere connection.
-		klog.V(4).Infof("Renewing NodeInfo %+v for node %q", nodeInfo, nodeName)
-		nodeInfo, err = nm.renewNodeInfo(nodeInfo, true)
-		if err != nil {
-			klog.Errorf("Error %q occurred while renewing NodeInfo for %q", err, nodeName)
-			return NodeInfo{}, err
-		}
-		nm.addNodeInfo(nodeName, nodeInfo)
-	}
-	return *nodeInfo, nil
 }
 
 func (nm *NodeManager) CredentialManager() *SecretCredentialManager {

staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere.go

@@ -28,6 +28,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"reflect"
 	"runtime"
 	"strings"
 	"sync"
@@ -98,6 +99,7 @@ type VSphere struct {
 	nodeManager          *NodeManager
 	vmUUID               string
 	isSecretInfoProvided bool
+	isSecretManaged      bool
 }
 
 // Represents a vSphere instance where one or more kubernetes nodes are running.
@@ -175,6 +177,8 @@ type VSphereConfig struct {
 		SecretName string `gcfg:"secret-name"`
 		// Secret Namespace where secret will be present that has vCenter credentials.
 		SecretNamespace string `gcfg:"secret-namespace"`
+		// Secret changes being ingnored for cloud resources
+		SecretNotManaged bool `gcfg:"secret-not-managed"`
 	}
 
 	VirtualCenter map[string]*VirtualCenterConfig
@@ -276,6 +280,15 @@ func (vs *VSphere) SetInformers(informerFactory informers.SharedInformerFactory)
 				VirtualCenter: make(map[string]*Credential),
 			},
 		}
+		if vs.isSecretManaged {
+			klog.V(4).Infof("Setting up secret informers for vSphere Cloud Provider")
+			secretInformer := informerFactory.Core().V1().Secrets().Informer()
+			secretInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
+				AddFunc:    vs.SecretAdded,
+				UpdateFunc: vs.SecretUpdated,
+			})
+			klog.V(4).Infof("Secret informers in vSphere cloud provider initialized")
+		}
 		vs.nodeManager.UpdateCredentialManager(secretCredentialManager)
 	}
 
@@ -530,6 +543,7 @@ func buildVSphereFromConfig(cfg VSphereConfig) (*VSphere, error) {
 			registeredNodes:    make(map[string]*v1.Node),
 		},
 		isSecretInfoProvided: isSecretInfoProvided,
+		isSecretManaged:      !cfg.Global.SecretNotManaged,
 		cfg:                  &cfg,
 	}
 	return &vs, nil
@@ -1502,6 +1516,54 @@ func (vs *VSphere) NodeDeleted(obj interface{}) {
 	}
 }
 
+// Notification handler when credentials secret is added.
+func (vs *VSphere) SecretAdded(obj interface{}) {
+	secret, ok := obj.(*v1.Secret)
+	if secret == nil || !ok {
+		klog.Warningf("Unrecognized secret object %T", obj)
+		return
+	}
+
+	if secret.Name != vs.cfg.Global.SecretName ||
+		secret.Namespace != vs.cfg.Global.SecretNamespace {
+		return
+	}
+
+	klog.V(4).Infof("secret added: %+v", obj)
+	vs.refreshNodesForSecretChange()
+}
+
+// Notification handler when credentials secret is updated.
+func (vs *VSphere) SecretUpdated(obj interface{}, newObj interface{}) {
+	oldSecret, ok := obj.(*v1.Secret)
+	if oldSecret == nil || !ok {
+		klog.Warningf("Unrecognized secret object %T", obj)
+		return
+	}
+
+	secret, ok := newObj.(*v1.Secret)
+	if secret == nil || !ok {
+		klog.Warningf("Unrecognized secret object %T", newObj)
+		return
+	}
+
+	if secret.Name != vs.cfg.Global.SecretName ||
+		secret.Namespace != vs.cfg.Global.SecretNamespace ||
+		reflect.DeepEqual(secret.Data, oldSecret.Data) {
+		return
+	}
+
+	klog.V(4).Infof("secret updated: %+v", newObj)
+	vs.refreshNodesForSecretChange()
+}
+
+func (vs *VSphere) refreshNodesForSecretChange() {
+	err := vs.nodeManager.refreshNodes()
+	if err != nil {
+		klog.Errorf("failed to rediscover nodes: %v", err)
+	}
+}
+
 func (vs *VSphere) NodeManager() (nodeManager *NodeManager) {
 	if vs == nil {
 		return nil

staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere_test.go

@@ -19,9 +19,11 @@ limitations under the License.
 package vsphere
 
 import (
+	"bytes"
 	"context"
 	"crypto/tls"
 	"crypto/x509"
+	"flag"
 	"io/ioutil"
 	"log"
 	"net/url"
@@ -44,8 +46,10 @@ import (
 	"github.com/vmware/govmomi/vim25/mo"
 	vmwaretypes "github.com/vmware/govmomi/vim25/types"
 	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/rand"
+	"k8s.io/klog/v2"
 
 	cloudprovider "k8s.io/cloud-provider"
 	"k8s.io/legacy-cloud-providers/vsphere/vclib"
@@ -831,6 +835,7 @@ func TestSecretVSphereConfig(t *testing.T) {
 		testName                 string
 		conf                     string
 		expectedIsSecretProvided bool
+		expectedSecretNotManaged bool
 		expectedUsername         string
 		expectedPassword         string
 		expectedError            error
@@ -985,6 +990,24 @@ func TestSecretVSphereConfig(t *testing.T) {
 			expectedIsSecretProvided: true,
 			expectedError:            nil,
 		},
+		{
+			testName: "SecretName and SecretNamespace with new configuration, but non-managed",
+			conf: `[Global]
+			server = 0.0.0.0
+			secret-name = "vccreds"
+			secret-namespace = "kube-system"
+			secret-not-managed = true
+			datacenter = us-west
+			[VirtualCenter "0.0.0.0"]
+			[Workspace]
+			server = 0.0.0.0
+			datacenter = us-west
+			folder = kubernetes
+			`,
+			expectedSecretNotManaged: true,
+			expectedIsSecretProvided: true,
+			expectedError:            nil,
+		},
 		{
 			testName: "SecretName and SecretNamespace with Username missing in new configuration",
 			conf: `[Global]
@@ -1098,6 +1121,11 @@ func TestSecretVSphereConfig(t *testing.T) {
 				}
 			}
 		}
+		if testcase.expectedSecretNotManaged && vs.isSecretManaged {
+			t.Fatalf("Expected secret being non-managed but vs.isSecretManaged: %v", vs.isSecretManaged)
+		} else if !testcase.expectedSecretNotManaged && !vs.isSecretManaged {
+			t.Fatalf("Expected secret being managed but vs.isSecretManaged: %v", vs.isSecretManaged)
+		}
 		// Check, if all the expected thumbprints are configured
 		for instanceName, expectedThumbprint := range testcase.expectedThumbprints {
 			instanceConfig, ok := vs.vsphereInstanceMap[instanceName]
@@ -1124,3 +1152,105 @@ func TestSecretVSphereConfig(t *testing.T) {
 		}
 	}
 }
+
+func fakeSecret(name, namespace, datacenter, user, password string) *v1.Secret {
+	return &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+		},
+		Data: map[string][]byte{
+			"vcenter." + datacenter + ".password": []byte(user),
+			"vcenter." + datacenter + ".username": []byte(password),
+		},
+	}
+}
+
+func TestSecretUpdated(t *testing.T) {
+	datacenter := "0.0.0.0"
+	secretName := "vccreds"
+	secretNamespace := "kube-system"
+	username := "test-username"
+	password := "test-password"
+	basicSecret := fakeSecret(secretName, secretNamespace, datacenter, username, password)
+
+	cfg, cleanup := configFromSim()
+	defer cleanup()
+
+	cfg.Global.User = username
+	cfg.Global.Password = password
+	cfg.Global.Datacenter = datacenter
+	cfg.Global.SecretName = secretName
+	cfg.Global.SecretNamespace = secretNamespace
+
+	vsphere, err := buildVSphereFromConfig(cfg)
+	if err != nil {
+		t.Fatalf("Should succeed when a valid config is provided: %s", err)
+	}
+	klog.Flush()
+
+	klog.InitFlags(nil)
+	flag.Set("logtostderr", "false")
+	flag.Set("alsologtostderr", "false")
+	flag.Set("v", "9")
+	flag.Parse()
+
+	testcases := []struct {
+		name            string
+		oldSecret       *v1.Secret
+		secret          *v1.Secret
+		expectOutput    bool
+		expectErrOutput bool
+	}{
+		{
+			name:         "Secrets are equal",
+			oldSecret:    basicSecret.DeepCopy(),
+			secret:       basicSecret.DeepCopy(),
+			expectOutput: false,
+		},
+		{
+			name:         "Secret with a different name",
+			oldSecret:    basicSecret.DeepCopy(),
+			secret:       fakeSecret("different", secretNamespace, datacenter, username, password),
+			expectOutput: false,
+		},
+		{
+			name:         "Secret with a different data",
+			oldSecret:    basicSecret.DeepCopy(),
+			secret:       fakeSecret(secretName, secretNamespace, datacenter, "...", "..."),
+			expectOutput: true,
+		},
+		{
+			name:            "Secret being nil",
+			oldSecret:       basicSecret.DeepCopy(),
+			secret:          nil,
+			expectOutput:    true,
+			expectErrOutput: true,
+		},
+	}
+
+	for _, testcase := range testcases {
+		t.Run(testcase.name, func(t *testing.T) {
+			buf := new(bytes.Buffer)
+			errBuf := new(bytes.Buffer)
+
+			klog.SetOutputBySeverity("INFO", buf)
+			klog.SetOutputBySeverity("WARNING", errBuf)
+
+			vsphere.SecretUpdated(testcase.oldSecret, testcase.secret)
+
+			klog.Flush()
+			if testcase.expectOutput && len(buf.String()) == 0 {
+				t.Fatalf("Expected log secret update for secrets:\nOld:\n\t%+v\nNew\n\t%+v", testcase.oldSecret, testcase.secret)
+			} else if !testcase.expectOutput && len(buf.String()) > 0 {
+				t.Fatalf("Unexpected log messages for secrets:\nOld:\n\t%+v\n\nNew:\n\t%+v\nMessage:%s", testcase.oldSecret, testcase.secret, buf.String())
+			}
+
+			if testcase.expectErrOutput && len(errBuf.String()) == 0 {
+				t.Fatalf("Expected log error output on secret update for secrets:\nOld:\n\t%+v\nNew\n\t%+v", testcase.oldSecret, testcase.secret)
+			} else if !testcase.expectErrOutput && len(errBuf.String()) > 0 {
+				t.Fatalf("Unexpected log error messages for secrets:\nOld:\n\t%+v\n\nNew:\n\t%+v\nMessage:%s", testcase.oldSecret, testcase.secret, errBuf.String())
+			}
+		})
+	}
+}