From 9e0555446238b2dfe45805babc2b6982565c293d Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Thu, 7 May 2020 00:41:39 +0200 Subject: [PATCH] Add ability for vSphere to reconnect to cached nodes on secret update or create - Refactor getNodeInfo to be more descriptive - Add log output testing for secret update - Ensure all nodes refreshed on secret change - Add config option to disable secret management --- .../legacy-cloud-providers/vsphere/BUILD | 1 + .../vsphere/nodemanager.go | 106 +++++++------- .../legacy-cloud-providers/vsphere/vsphere.go | 62 +++++++++ .../vsphere/vsphere_test.go | 130 ++++++++++++++++++ 4 files changed, 242 insertions(+), 57 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/vsphere/BUILD b/staging/src/k8s.io/legacy-cloud-providers/vsphere/BUILD index c2eed976996..f0dfe0f1d19 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/vsphere/BUILD +++ b/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", ], ) diff --git a/staging/src/k8s.io/legacy-cloud-providers/vsphere/nodemanager.go b/staging/src/k8s.io/legacy-cloud-providers/vsphere/nodemanager.go index 14b9fc01891..ff5d4df7bfb 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/vsphere/nodemanager.go +++ b/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 { - 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 + return nm.getRefreshedNodeInfo(nodeName) } // 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 - 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 + return nm.getRefreshedNodeInfo(convertToK8sType(node.Name)) } func (nm *NodeManager) CredentialManager() *SecretCredentialManager { diff --git a/staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere.go b/staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere.go index 125c6f17b12..2ef2a30833c 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere.go +++ b/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 diff --git a/staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere_test.go b/staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere_test.go index 8ad8f3ea960..c5ea17ba963 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere_test.go +++ b/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()) + } + }) + } +}