Use the same session of SOAP client when VC is bigger than 8.0U3

[rikatz]

Description

On vCenter versions bigger than 8.0U3 there is no need anymore to do a new Rest Login. Instead, the same token/cookie from the Soap client can be reused.

The current change makes the rest.NewClient verify if the parent soap client was created against a vCenter bigger than 8U3 and reuse the session, and also turns Login() a noop in the same case.

How Has This Been Tested?

Did some internal tests with the code calling library and tags API. More tests are pending

Guidelines

Please read and follow the CONTRIBUTION guidelines of this project.

[dougm]

We have a tiny version parser already: https://github.com/vmware/govmomi/blob/main/cli/flags/version.go#L32
It could be moved to another package outside of cli, but should be all that's needed for this use case.
Tho we may not need to check version at all, see comment below.

[tenthirtyam]

hashicorp/go-version is also one I recommend in the event you need and external package.

[rikatz]

yeah, but using the internal version should be fine for us I guess. We now have the version that can be semver but can also be 9.0.0.0 so probably our internal parser should deal with it.

I will check on moving this package to another folder (on a separate PR) and come back to this one.

[tenthirtyam]

We should consider - future of course - if we need our own external library based on our versioning scheme that can be used by our projects. Happy to hit this one up later. LMK.

[dougm]

Not sure about changing the behavior of NewClient and Login. Thinking about the various ways existing applications handle the need to re-authenticate and not wanting to break them. Using SessionID() here does not account for that session becoming invalid. Untested, but I'm thinking like the patch below. So if an app/caller is aware of this U3 feature, they simply don't call the Login/Logout methods and every rest API call will pull the current soap session from the soap.Client.Jar. What do you think?

diff --git a/vapi/rest/client.go b/vapi/rest/client.go
index 048b65f3..6993bc70 100644
--- a/vapi/rest/client.go
+++ b/vapi/rest/client.go
@@ -42,6 +42,8 @@ type Client struct {
 
 	*soap.Client
 	sessionID string
+
+	soapSession func() *soap.HeaderElement
 }
 
 // Session information
@@ -66,7 +68,7 @@ func (m *LocalizableMessage) Error() string {
 func NewClient(c *vim25.Client) *Client {
 	sc := c.Client.NewServiceClient(Path, "")
 
-	return &Client{Client: sc}
+	return &Client{Client: sc, soapSession: c.SessionCookie}
 }
 
 // SessionID is set by calling Login() or optionally with the given id param
@@ -76,7 +78,13 @@ func (c *Client) SessionID(id ...string) string {
 	if len(id) != 0 {
 		c.sessionID = id[0]
 	}
-	return c.sessionID
+	if c.sessionID != "" {
+		return c.sessionID
+	}
+	if c := c.soapSession(); c != nil {
+		return c.Value
+	}
+	return ""
 }
 
 type marshaledClient struct {

[dougm]

Another version of the patch (also untested), but we should just share the CookieJar, can't think of why we didn't do that originally. Then you can use the already embedded soap.Client.SessionCookie:

diff --git a/vapi/rest/client.go b/vapi/rest/client.go
index 048b65f3..4d59048c 100644
--- a/vapi/rest/client.go
+++ b/vapi/rest/client.go
@@ -76,7 +76,13 @@ func (c *Client) SessionID(id ...string) string {
 	if len(id) != 0 {
 		c.sessionID = id[0]
 	}
-	return c.sessionID
+	if c.sessionID != "" {
+		return c.sessionID
+	}
+	if c := c.SessionCookie(); c != nil {
+		return c.Value
+	}
+	return ""
 }
 
 type marshaledClient struct {
diff --git a/vim25/soap/client.go b/vim25/soap/client.go
index b7ff99f5..d2f12b09 100644
--- a/vim25/soap/client.go
+++ b/vim25/soap/client.go
@@ -260,8 +260,8 @@ func (c *Client) newServiceClientWithTransport(path string, namespace string, t
 	}
 	c.hostsMu.Unlock()
 
-	// Copy the cookies
-	client.Client.Jar.SetCookies(u, c.Client.Jar.Cookies(u))
+	// Share the cookie Jar, it is thread safe
+	client.Client.Jar = c.Client.Jar
 
 	// Copy any query params (e.g. GOVMOMI_TUNNEL_PROXY_PORT used in testing)
 	client.u.RawQuery = vc.RawQuery

[rikatz]

I was thinking about it. My bigger concern is about situations like a CSI/CPI case. They may connect to multiple vCenters, so in that case still for those programs to be aware or not, they would need to check the VC version before doing Login/Logout, right? I can test if just changing the approach as suggested would work.

[dougm]

vim25.Client and vCenter instance are 1:1, same with rest.Client, don't see multiple VCs being a problem here.
CSI/CPI/etc apps would only need be aware of VC version if they choose to opt-into this session sharing.
Otherwise the existing pattern of Login+Logout to the rest endpoint will continue to work the same way.

[rikatz]

folks, I've been on vacation and am coming back kind of now. will be back to this PR soon, sorry for the delay

[github-actions]

This pull request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Mark as fresh by adding the comment /remove-lifecycle stale.