Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] ssoadmin.NewClient() method does NOT inherit transport setting from c *vim25.Client passed in. #3039

Closed
kmahyyg opened this issue Jan 28, 2023 · 2 comments · Fixed by #3161

Comments

@kmahyyg
Copy link

kmahyyg commented Jan 28, 2023

Describe the bug

ssoadmin.NewClient() create a new *soap.Client without any reuse of passed in *vim25.Client.

To Reproduce

Steps to reproduce the behavior:

  1. Set a proxy using http.Transport like this sc.DefaultTransport().Proxy = http.ProxyURL(proxyServerAddr) when trying to login using default *vim25.Client
  2. Call ssoadmin.NewClient(ctx context.Context, c *vim25.Client) (*Client, error) with a proxy-enabled *vim25.Client that has been set up using above method.
  3. /go/pkg/mod/github.com/vmware/govmomi@v0.30.0/ssoadmin/client.go:78 created a new *soap.Client without any inheritance of the param c passed in. Thus, Proxy function will not work.

Expected behavior

/go/pkg/mod/github.com/vmware/govmomi@v0.30.0/ssoadmin/client.go:78 should reuse c *vim25.Client if possible.

Affected version

govmomi@v0.30.0

Screenshots/Debug Output

Normally and expected:

20230128-184146-GoLand-001370@2x

Unexpected, proxy function in transport is not reused:

20230128-184347-GoLand-001372@2x

Additional context

A temporary solution here might be changing the whole default Transport value of http.Client, may be related to golang/go#39299 .

@github-actions
Copy link
Contributor

Howdy 🖐   kmahyyg ! Thank you for your interest in this project. We value your feedback and will respond soon.

If you want to contribute to this project, please make yourself familiar with the CONTRIBUTION guidelines.

@dougm
Copy link
Member

dougm commented Apr 18, 2023

I don't recall why the entire transport wasn't shared, but the (untested) change below should work. Not sure this is the right fix, but certainly we should include the proxy settings here.

modified   vim25/soap/client.go
@@ -199,10 +199,7 @@ func (c *Client) NewServiceClient(path string, namespace string) *Client {
 
 	client := NewClient(u, c.k)
 	client.Namespace = "urn:" + namespace
-	client.DefaultTransport().TLSClientConfig = c.DefaultTransport().TLSClientConfig
-	if cert := c.Certificate(); cert != nil {
-		client.SetCertificate(*cert)
-	}
+	client.Client.Transport = c.Client.Transport // or c.Client.Transport.Clone()
 
 	// Copy the trusted thumbprints
 	c.hostsMu.Lock()

dougm added a commit to dougm/govmomi that referenced this issue Jun 27, 2023
dougm added a commit to dougm/govmomi that referenced this issue Jun 27, 2023
dougm added a commit to dougm/govmomi that referenced this issue Jun 27, 2023
priyanka19-98 pushed a commit to priyanka19-98/govmomi that referenced this issue Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants