Refactor tag manager rest client (kubernetes-sigs#3412)

Force re-login of rest session in case of expired session

Author: BernardMC

hack/release.sh

@@ -147,7 +147,9 @@ function build_driver_images_linux() {
 
 function build_syncer_image_linux() {
   echo "building ${SYNCER_IMAGE_NAME}:${VERSION} for linux"
-  docker buildx build --platform "linux/$ARCH"\
+  docker buildx build \
+      --platform "linux/$ARCH" \
+      --output "${LINUX_IMAGE_OUTPUT}" \
       -f images/syncer/Dockerfile \
       -t "${SYNCER_IMAGE_NAME}":"${VERSION}" \
       --build-arg "VERSION=${VERSION}" \

pkg/common/cns-lib/vsphere/tagmanager.go

@@ -0,0 +1,29 @@
+package vsphere
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/vmware/govmomi/vapi/tags"
+	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/logger"
+)
+
+// GetTagManager returns tagManager connected to given VirtualCenter.
+func (vc *VirtualCenter) GetTagManager(ctx context.Context) (*tags.Manager, error) {
+	log := logger.GetLogger(ctx)
+	// Validate input.
+	if vc == nil || vc.Client == nil || vc.Client.Client == nil {
+		return nil, fmt.Errorf("vCenter not initialized")
+	}
+
+	if err := vc.Connect(ctx); err != nil {
+		return nil, fmt.Errorf("error connecting to VC: %w", err)
+	}
+
+	vc.tagManager = tags.NewManager(vc.RestClient)
+	if vc.tagManager == nil {
+		return nil, fmt.Errorf("failed to create a tagManager")
+	}
+	log.Infof("New tag manager with useragent '%s'", vc.tagManager.UserAgent)
+	return vc.tagManager, nil
+}

pkg/common/cns-lib/vsphere/utils.go

@@ -2,22 +2,15 @@ package vsphere
 
 import (
 	"context"
-	"crypto/tls"
-	"encoding/pem"
 	"errors"
 	"fmt"
-	"net/url"
 	"reflect"
 	"strconv"
 	"strings"
 
 	"github.com/davecgh/go-spew/spew"
 	"github.com/vmware/govmomi/cns"
 	cnstypes "github.com/vmware/govmomi/cns/types"
-	"github.com/vmware/govmomi/sts"
-	"github.com/vmware/govmomi/vapi/rest"
-	"github.com/vmware/govmomi/vapi/tags"
-	"github.com/vmware/govmomi/vim25"
 	"github.com/vmware/govmomi/vim25/soap"
 	"github.com/vmware/govmomi/vim25/types"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/config"
@@ -304,62 +297,6 @@ func CompareKubernetesMetadata(ctx context.Context, k8sMetaData *cnstypes.CnsKub
 	return labelsMatch
 }
 
-// Signer decodes the certificate and private key and returns SAML token needed
-// for authentication.
-func signer(ctx context.Context, client *vim25.Client, username string, password string) (*sts.Signer, error) {
-	pemBlock, _ := pem.Decode([]byte(username))
-	if pemBlock == nil {
-		return nil, nil
-	}
-	certificate, err := tls.X509KeyPair([]byte(username), []byte(password))
-	if err != nil {
-		return nil, fmt.Errorf("failed to load X509 key pair. Error: %+v", err)
-	}
-	tokens, err := sts.NewClient(ctx, client)
-	if err != nil {
-		return nil, fmt.Errorf("failed to create STS client. err: %+v", err)
-	}
-	req := sts.TokenRequest{
-		Certificate: &certificate,
-		Delegatable: true,
-	}
-	signer, err := tokens.Issue(ctx, req)
-	if err != nil {
-		return nil, fmt.Errorf("failed to issue SAML token. err: %+v", err)
-	}
-	return signer, nil
-}
-
-// GetTagManager returns tagManager connected to given VirtualCenter.
-func GetTagManager(ctx context.Context, vc *VirtualCenter) (*tags.Manager, error) {
-	log := logger.GetLogger(ctx)
-	// Validate input.
-	if vc == nil || vc.Client == nil || vc.Client.Client == nil {
-		return nil, fmt.Errorf("vCenter not initialized")
-	}
-
-	restClient := rest.NewClient(vc.Client.Client)
-	signer, err := signer(ctx, vc.Client.Client, vc.Config.Username, vc.Config.Password)
-	if err != nil {
-		return nil, fmt.Errorf("failed to create the Signer. Error: %v", err)
-	}
-	if signer == nil {
-		user := url.UserPassword(vc.Config.Username, vc.Config.Password)
-		err = restClient.Login(ctx, user)
-	} else {
-		err = restClient.LoginByToken(restClient.WithSigner(ctx, signer))
-	}
-	if err != nil {
-		return nil, fmt.Errorf("failed to login for the rest client. Error: %v", err)
-	}
-	tagManager := tags.NewManager(restClient)
-	if tagManager == nil {
-		return nil, fmt.Errorf("failed to create a tagManager")
-	}
-	log.Infof("New tag manager with useragent '%s'", tagManager.UserAgent)
-	return tagManager, nil
-}
-
 // GetCandidateDatastoresInClusters gets the shared datastores and vSAN-direct
 // managed datastores of given VC clusters from GetCandidateDatastoresInCluster and
 // returns a map of clusterID -> array of datastores

pkg/common/cns-lib/vsphere/virtualcenter.go

@@ -37,6 +37,8 @@ import (
 	"github.com/vmware/govmomi/property"
 	"github.com/vmware/govmomi/session"
 	"github.com/vmware/govmomi/sts"
+	"github.com/vmware/govmomi/vapi/rest"
+	"github.com/vmware/govmomi/vapi/tags"
 	"github.com/vmware/govmomi/vim25"
 	"github.com/vmware/govmomi/vim25/mo"
 	"github.com/vmware/govmomi/vim25/soap"
@@ -68,6 +70,8 @@ type VirtualCenter struct {
 	Config *VirtualCenterConfig
 	// Client represents the govmomi client instance for the connection.
 	Client *govmomi.Client
+	// RestClient represents the govmomi rest client
+	RestClient *rest.Client
 	// PbmClient represents the govmomi PBM Client instance.
 	PbmClient *pbm.Client
 	// CnsClient represents the CNS client instance.
@@ -76,6 +80,9 @@ type VirtualCenter struct {
 	VsanClient *vsan.Client
 	// VslmClient represents the Vslm client instance.
 	VslmClient *vslm.Client
+	// tagManager represents the tagmanager client instance.
+	tagManager *tags.Manager
+
 	// ClientMutex is used for exclusive connection creation.
 	ClientMutex *sync.Mutex
 }
@@ -151,7 +158,7 @@ type VirtualCenterConfig struct {
 }
 
 // NewClient creates a new govmomi Client instance.
-func (vc *VirtualCenter) NewClient(ctx context.Context, useragent string) (*govmomi.Client, error) {
+func (vc *VirtualCenter) NewClient(ctx context.Context, useragent string) (*govmomi.Client, *rest.Client, error) {
 	log := logger.GetLogger(ctx)
 	if vc.Config.Scheme == "" {
 		vc.Config.Scheme = DefaultScheme
@@ -160,14 +167,14 @@ func (vc *VirtualCenter) NewClient(ctx context.Context, useragent string) (*govm
 	url, err := soap.ParseURL(net.JoinHostPort(vc.Config.Host, strconv.Itoa(vc.Config.Port)))
 	if err != nil {
 		log.Errorf("failed to parse URL %s with err: %v", url, err)
-		return nil, err
+		return nil, nil, err
 	}
 
 	soapClient := soap.NewClient(url, vc.Config.Insecure)
 	if len(vc.Config.CAFile) > 0 && !vc.Config.Insecure {
 		if err := soapClient.SetRootCAs(vc.Config.CAFile); err != nil {
 			log.Errorf("failed to load CA file: %v", err)
-			return nil, err
+			return nil, nil, err
 		}
 	} else if len(vc.Config.Thumbprint) > 0 && !vc.Config.Insecure {
 		soapClient.SetThumbprint(url.Host, vc.Config.Thumbprint)
@@ -179,36 +186,38 @@ func (vc *VirtualCenter) NewClient(ctx context.Context, useragent string) (*govm
 	vimClient, err := vim25.NewClient(ctx, soapClient)
 	if err != nil {
 		log.Errorf("failed to create new client with err: %v", err)
-		return nil, err
+		return nil, nil, err
 	}
 	err = vimClient.UseServiceVersion("vsan")
 	if err != nil && vc.Config.Host != "127.0.0.1" {
 		// Skipping error for simulator connection for unit tests.
 		log.Errorf("Failed to set vimClient service version to vsan. err: %v", err)
-		return nil, err
+		return nil, nil, err
 	}
 	vimClient.UserAgent = useragent
 	client := &govmomi.Client{
 		Client:         vimClient,
 		SessionManager: session.NewManager(vimClient),
 	}
 
-	err = vc.login(ctx, client)
+	restClient := rest.NewClient(client.Client)
+
+	err = vc.login(ctx, client, restClient)
 	if err != nil {
 		log.Errorf("failed to login to vc. err: %v", err)
-		return nil, err
+		return nil, nil, err
 	}
 
 	s, err := client.SessionManager.UserSession(ctx)
 	if err != nil {
 		log.Errorf("failed to get UserSession. err: %v", err)
-		return nil, err
+		return nil, nil, err
 	}
 	// Refer to this issue - https://github.com/vmware/govmomi/issues/2922
 	// When Session Manager -> UserSession can return nil user session with nil error
 	// so handling the case for nil session.
 	if s == nil {
-		return nil, errors.New("nil session obtained from session manager")
+		return nil, nil, errors.New("nil session obtained from session manager")
 	}
 	log.Infof("New session ID for '%s' = %s", s.UserName, s.Key)
 
@@ -217,19 +226,22 @@ func (vc *VirtualCenter) NewClient(ctx context.Context, useragent string) (*govm
 	}
 	rt := vim25.Retry(client.RoundTripper, vim25.TemporaryNetworkError(vc.Config.RoundTripperCount))
 	client.RoundTripper = &MetricRoundTripper{"soap", rt}
-	return client, nil
+	return client, restClient, nil
 }
 
 // login calls SessionManager.LoginByToken if certificate and private key are
 // configured. Otherwise, calls SessionManager.Login with user and password.
-func (vc *VirtualCenter) login(ctx context.Context, client *govmomi.Client) error {
+func (vc *VirtualCenter) login(ctx context.Context, client *govmomi.Client, restClient *rest.Client) error {
 	log := logger.GetLogger(ctx)
 	var err error
 
 	b, _ := pem.Decode([]byte(vc.Config.Username))
 	if b == nil {
-		return client.SessionManager.Login(ctx,
-			neturl.UserPassword(vc.Config.Username, vc.Config.Password))
+		if err := client.SessionManager.Login(ctx, neturl.UserPassword(vc.Config.Username, vc.Config.Password)); err != nil {
+			log.Errorf("error logging soap client: %v", err)
+			return err
+		}
+		return restClient.Login(ctx, neturl.UserPassword(vc.Config.Username, vc.Config.Password))
 	}
 
 	cert, err := tls.X509KeyPair([]byte(vc.Config.Username), []byte(vc.Config.Password))
@@ -246,6 +258,7 @@ func (vc *VirtualCenter) login(ctx context.Context, client *govmomi.Client) erro
 
 	req := sts.TokenRequest{
 		Certificate: &cert,
+		Delegatable: true,
 	}
 
 	signer, err := tokens.Issue(ctx, req)
@@ -255,7 +268,11 @@ func (vc *VirtualCenter) login(ctx context.Context, client *govmomi.Client) erro
 	}
 
 	header := soap.Header{Security: signer}
-	return client.SessionManager.LoginByToken(client.Client.WithHeader(ctx, header))
+	if err := client.SessionManager.LoginByToken(client.Client.WithHeader(ctx, header)); err != nil {
+		return err
+	}
+
+	return restClient.LoginByToken(restClient.WithSigner(ctx, signer))
 }
 
 // Connect establishes a new connection with vSphere with updated credentials.
@@ -279,6 +296,14 @@ func (vc *VirtualCenter) Connect(ctx context.Context) error {
 					log.Errorf("Could not logout of VC session. Error: %v", logoutErr)
 				}
 			}
+			if vc.RestClient != nil {
+				logoutErr := vc.RestClient.Logout(ctx)
+				if logoutErr != nil {
+					// TODO: On vSphere U3, with a shared session this may return an error as logging
+					// out from Soap also logs out from rest.
+					log.Errorf("Could not logout of VC rest session. Error: %v", logoutErr)
+				}
+			}
 		}()
 	}
 	return err
@@ -295,21 +320,22 @@ func (vc *VirtualCenter) connect(ctx context.Context) error {
 		log.Errorf("failed to get useragent for vCenter session. error: %+v", err)
 		return err
 	}
-	if vc.Client == nil {
+	if vc.Client == nil || vc.RestClient == nil {
 		if vc.Config.ReloadVCConfigForNewClient {
 			err = ReadVCConfigs(ctx, vc)
 			if err != nil {
 				return err
 			}
 		}
 		log.Infof("VirtualCenter.connect() creating new client")
-		if vc.Client, err = vc.NewClient(ctx, useragent); err != nil {
+		if vc.Client, vc.RestClient, err = vc.NewClient(ctx, useragent); err != nil {
 			log.Errorf("failed to create govmomi client with err: %v", err)
 			if !vc.Config.Insecure {
 				log.Errorf("failed to connect to vCenter using CA file: %q", vc.Config.CAFile)
 			}
 			return err
 		}
+
 		log.Infof("VirtualCenter.connect() successfully created new client")
 		return nil
 	}
@@ -319,10 +345,25 @@ func (vc *VirtualCenter) connect(ctx context.Context) error {
 	// SessionMgr.UserSession(ctx) retrieves and returns the SessionManager's
 	// CurrentSession field. Nil is returned if the session is not
 	// authenticated or timed out.
-	if userSession, err := sessionMgr.UserSession(ctx); err != nil {
+	shouldLogin := false
+
+	userSession, err := sessionMgr.UserSession(ctx)
+	if err != nil {
 		log.Errorf("failed to obtain user session with err: %v", err)
-		return err
-	} else if userSession != nil {
+		// Login again if session is invalid
+		shouldLogin = true
+		//return err <- Check if we really want to return error here or follow with re-login
+	}
+
+	restSession, err := vc.RestClient.Session(ctx)
+	if err != nil {
+		log.Errorf("failed to obtain rest user session with err: %v", err)
+		// Login again if session is invalid
+		shouldLogin = true
+	}
+
+	// No need to re-login
+	if userSession != nil && restSession != nil && !shouldLogin {
 		return nil
 	}
 
@@ -335,6 +376,14 @@ func (vc *VirtualCenter) connect(ctx context.Context) error {
 		}
 	}
 
+	if vc.RestClient != nil {
+		// TODO: On U3 shared session this may return an error, if the Soap logout
+		// happened correctly, but can be safely ignored
+		if err := vc.RestClient.Logout(ctx); err != nil {
+			log.Errorf("failed to logout current rest session. still clearing idle sessions. err: %v", err)
+		}
+	}
+
 	// If session has expired, create a new instance.
 	log.Infof("Creating a new client session as the existing one isn't valid or not authenticated")
 	if vc.Config.ReloadVCConfigForNewClient {
@@ -343,7 +392,7 @@ func (vc *VirtualCenter) connect(ctx context.Context) error {
 			return err
 		}
 	}
-	if vc.Client, err = vc.NewClient(ctx, useragent); err != nil {
+	if vc.Client, vc.RestClient, err = vc.NewClient(ctx, useragent); err != nil {
 		log.Errorf("failed to create govmomi client with err: %v", err)
 		if !vc.Config.Insecure {
 			log.Errorf("failed to connect to vCenter using CA file: %q", vc.Config.CAFile)
@@ -382,6 +431,12 @@ func (vc *VirtualCenter) connect(ctx context.Context) error {
 		}
 		vc.VsanClient.RoundTripper = &MetricRoundTripper{"vsan", vc.VsanClient.RoundTripper}
 	}
+
+	// Recreate the Tag Manager client if created using timed out Rest client
+	if vc.tagManager != nil {
+		vc.tagManager = tags.NewManager(vc.RestClient)
+	}
+
 	return nil
 }
 
@@ -485,7 +540,14 @@ func (vc *VirtualCenter) Disconnect(ctx context.Context) error {
 		log.Errorf("failed to logout with err: %v", err)
 		return err
 	}
+
+	// We don't return an error here because logging out from Rest failing
+	// can happen on VC shared sessions
+	if err := vc.RestClient.Logout(ctx); err != nil {
+		log.Errorf("failed to logout rest with err: %v", err)
+	}
 	vc.Client = nil
+	vc.RestClient = nil
 	return nil
 }