Repository: vmware/govmomi
Pull request #2872 (2022-06-10 08:15:37)

Add versioned user-agent header
## Description

If a user does not provide a custom `user-agent` via `client.UserAgent` a default request header `user-agent` will be constructed using `ClientName/ClientVersion (GOVERSION;GOOS;GOARCH)` syntax, e.g. `govmomi/0.28.0 (go1.18.3;linux;amd64)` and added to each outgoing HTTP request (SOAP/VAPI).

Adds a new `version.go` file in `internal/package`. The version number gets bumped on every release (see TODO below).

I decided to put this change in `client.Do` because there's multiple ways to construct a `Client`. `Do` is the place where we have full control over the new behavior without touching existing constructors, etc.

See comments in the PR review section for further details on the proposed implementation.

Not considering this a breaking change:

- currently we don't set a `user-agent` header in the library (`govc` does), i.e. the Go `net/http` standard header is used which I don't think users rely on as it is generic
- a custom `user-agent` can be supplied via `client.UserAgent`, retaining the current behavior and what some users are doing (see issue)

Closes: #2693
Signed-off-by: Michael Gasch <mgasch@vmware.com>

TODO:

- [ ] Update release automation to sync `SDKVersion` (separate PR)

## Type of change

Please mark options that are relevant:

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
- [ ] This change requires a documentation update
- [ ] Build related change

## How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide
instructions so we can reproduce. If applicable, please also list any relevant
details for your test configuration.

- [x] Added unit tests

## Checklist:

- [x] My code follows the CONTRIBUTION [guidelines](./../CONTRIBUTING.md) of
  this project
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged

embano1: @karaatanassov @dougm keeping this as a draft to gather your feedback

Patch:
diff --git a/internal/version/version.go b/internal/version/version.go
new file mode 100644
index 00000000..011f379d
--- /dev/null
+++ b/internal/version/version.go
@@ -0,0 +1,25 @@
+/*
+Copyright (c) 2014-2022 VMware, Inc. All Rights Reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package version
+
+const (
+	// ClientName is the name of this SDK
+	ClientName = "govmomi"
+
+	// ClientVersion is the version of this SDK
+	ClientVersion = "master"
+)
diff --git a/simulator/folder.go b/simulator/folder.go
index 4f8ef144..cbbfd3aa 100644
--- a/simulator/folder.go
+++ b/simulator/folder.go
@@ -200,7 +200,7 @@ func (f *Folder) CreateFolder(ctx *Context, c *types.CreateFolder) soap.HasFault
 	r := &methods.CreateFolderBody{}
 
 	if folderHasChildType(&f.Folder, "Folder") {
-		name := escapeSpecialCharacters(c.Name)
+		name := f.escapeSpecialCharacters(c.Name)
 
 		if obj := ctx.Map.FindByName(name, f.ChildEntity); obj != nil {
 			r.Fault_ = Fault("", &types.DuplicateName{
@@ -228,7 +228,7 @@ func (f *Folder) CreateFolder(ctx *Context, c *types.CreateFolder) soap.HasFault
 	return r
 }
 
-func escapeSpecialCharacters(name string) string {
+func (f *Folder) escapeSpecialCharacters(name string) string {
 	name = strings.ReplaceAll(name, `%`, strings.ToLower(url.QueryEscape(`%`)))
 	name = strings.ReplaceAll(name, `/`, strings.ToLower(url.QueryEscape(`/`)))
 	name = strings.ReplaceAll(name, `\`, strings.ToLower(url.QueryEscape(`\`)))
@@ -367,15 +367,9 @@ func hostsWithDatastore(hosts []types.ManagedObjectReference, path string) []typ
 }
 
 func (c *createVM) Run(task *Task) (types.AnyType, types.BaseMethodFault) {
-	config := &c.req.Config
-	// escape special characters in vm name
-	if config.Name != escapeSpecialCharacters(config.Name) {
-		deepCopy(c.req.Config, config)
-		config.Name = escapeSpecialCharacters(config.Name)
-	}
-
 	vm, err := NewVirtualMachine(c.ctx, c.Folder.Self, &c.req.Config)
 	if err != nil {
+		folderRemoveChild(c.ctx, &c.Folder.Folder, vm)
 		return nil, err
 	}
 
diff --git a/simulator/virtual_machine.go b/simulator/virtual_machine.go
index 1da639ed..f0a7105c 100644
--- a/simulator/virtual_machine.go
+++ b/simulator/virtual_machine.go
@@ -166,7 +166,6 @@ func NewVirtualMachine(ctx *Context, parent types.ManagedObjectReference, spec *
 	vm.Summary.OverallStatus = types.ManagedEntityStatusGreen
 	vm.ConfigStatus = types.ManagedEntityStatusGreen
 
-	// put vm in the folder only if no errors occurred
 	f, _ := asFolderMO(folder)
 	folderPutChild(ctx, f, vm)
 
diff --git a/vim25/soap/client.go b/vim25/soap/client.go
index 424c20ce..5c63bcc9 100644
--- a/vim25/soap/client.go
+++ b/vim25/soap/client.go
@@ -37,10 +37,12 @@ import (
 	"path/filepath"
 	"reflect"
 	"regexp"
+	"runtime"
 	"strings"
 	"sync"
 	"time"
 
+	"github.com/vmware/govmomi/internal/version"
 	"github.com/vmware/govmomi/vim25/progress"
 	"github.com/vmware/govmomi/vim25/types"
 	"github.com/vmware/govmomi/vim25/xml"
@@ -58,6 +60,15 @@ const (
 	SessionCookieName = "vmware_soap_session"
 )
 
+// defaultUserAgent is the default user agent string, e.g.
+// "govmomi/0.28.0 (go1.18.3;linux;amd64)"
+var defaultUserAgent = fmt.Sprintf(
+	"%s/%s (%s)",
+	version.ClientName,
+	version.ClientVersion,
+	strings.Join([]string{runtime.Version(), runtime.GOOS, runtime.GOARCH}, ";"),
+)
+
 type Client struct {
 	http.Client
 
@@ -503,10 +514,13 @@ func (c *Client) Do(ctx context.Context, req *http.Request, f func(*http.Respons
 		defer d.done()
 	}
 
-	if c.UserAgent != "" {
-		req.Header.Set(`User-Agent`, c.UserAgent)
+	// use default
+	if c.UserAgent == "" {
+		c.UserAgent = defaultUserAgent
 	}
 
+	req.Header.Set(`User-Agent`, c.UserAgent)
+
 	ext := ""
 	if d.enabled() {
 		ext = d.debugRequest(req)


Test patch:
diff --git a/simulator/virtual_machine_test.go b/simulator/virtual_machine_test.go
index 4a65fd3b..7f180cbd 100644
--- a/simulator/virtual_machine_test.go
+++ b/simulator/virtual_machine_test.go
@@ -30,7 +30,6 @@ import (
 	"github.com/vmware/govmomi/property"
 	"github.com/vmware/govmomi/simulator/esx"
 	"github.com/vmware/govmomi/task"
-	"github.com/vmware/govmomi/vim25"
 	"github.com/vmware/govmomi/vim25/types"
 )
 
@@ -265,74 +264,6 @@ func TestCreateVm(t *testing.T) {
 	}
 }
 
-func TestCreateVmWithSpecialCharaters(t *testing.T) {
-	tests := []struct {
-		name     string
-		expected string
-	}{
-		{`/`, `%2f`},
-		{`\`, `%5c`},
-		{`%`, `%25`},
-		// multiple special characters
-		{`%%`, `%25%25`},
-		// slash-separated name
-		{`foo/bar`, `foo%2fbar`},
-	}
-
-	for _, test := range tests {
-		m := ESX()
-
-		Test(func(ctx context.Context, c *vim25.Client) {
-			finder := find.NewFinder(c, false)
-
-			dc, err := finder.DefaultDatacenter(ctx)
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			finder.SetDatacenter(dc)
-			folders, err := dc.Folders(ctx)
-			if err != nil {
-				t.Fatal(err)
-			}
-			vmFolder := folders.VmFolder
-
-			ds, err := finder.DefaultDatastore(ctx)
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			spec := types.VirtualMachineConfigSpec{
-				Name: test.name,
-				Files: &types.VirtualMachineFileInfo{
-					VmPathName: fmt.Sprintf("[%s]", ds.Name()),
-				},
-			}
-
-			pool := object.NewResourcePool(c, esx.ResourcePool.Self)
-
-			task, err := vmFolder.CreateVM(ctx, spec, pool, nil)
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			info, err := task.WaitForResult(ctx, nil)
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			vm := object.NewVirtualMachine(c, info.Result.(types.ManagedObjectReference))
-			name, err := vm.ObjectName(ctx)
-			if err != nil {
-				t.Fatal(err)
-			}
-			if name != test.expected {
-				t.Errorf("expected %s, got %s", test.expected, name)
-			}
-		}, m)
-	}
-}
-
 func TestReconfigVmDevice(t *testing.T) {
 	ctx := context.Background()
 
diff --git a/vim25/soap/client_test.go b/vim25/soap/client_test.go
index 24d51044..06e9e733 100644
--- a/vim25/soap/client_test.go
+++ b/vim25/soap/client_test.go
@@ -17,11 +17,71 @@ limitations under the License.
 package soap
 
 import (
+	"context"
+	"fmt"
+	"net/http"
 	"net/url"
 	"os"
 	"testing"
 )
 
+type mockRT struct{}
+
+func (m mockRT) RoundTrip(req *http.Request) (*http.Response, error) {
+	var res http.Response
+	res.Header = req.Header.Clone()
+	return &res, nil
+}
+
+func TestUserAgent(t *testing.T) {
+	tests := []struct {
+		name  string
+		agent string
+	}{
+		{name: "default agent", agent: ""},
+		{name: "custom agent", agent: "govmomi-test/0.0.0"},
+	}
+
+	const rawURL = "https://vcenter.local"
+	u, err := url.Parse(rawURL)
+	if err != nil {
+		t.Fatalf("parse url: %v", err)
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			c := NewClient(u, false)
+			c.Transport = &mockRT{}
+
+			req, err := http.NewRequest(http.MethodPost, rawURL, nil)
+			if err != nil {
+				t.Fatalf("create request: %v", err)
+			}
+
+			if test.agent != "" {
+				c.UserAgent = test.agent
+			}
+
+			if err = c.Do(context.Background(), req, func(response *http.Response) error {
+				got := response.Header.Get("user-agent")
+				want := func() string {
+					if test.agent == "" {
+						return defaultUserAgent
+					}
+					return test.agent
+				}
+
+				if got != want() {
+					return fmt.Errorf("user-agent header mismatch: got=%s want=%s", got, want())
+				}
+				return nil
+			}); err != nil {
+				t.Errorf("user-agent header validation failed: %v", err)
+			}
+		})
+	}
+}
+
 func TestSplitHostPort(t *testing.T) {
 	tests := []struct {
 		url  string
@@ -68,7 +128,6 @@ func TestInvalidRootCAPath(t *testing.T) {
 
 func TestValidRootCAs(t *testing.T) {
 	err := setCAsOnClient("fixtures/valid-cert.pem")
-
 	if err != nil {
 		t.Fatalf("Err should not have occurred: %#v", err)
 	}