Repository: vmware/govmomi
Pull request #2746 (2022-02-09 20:24:58)

Ignore concurrent deletes in `GetCategories()`
## Description

`GetCategories()` would fail if a category was deleted between the initial `ListCategories()` and `GetCategory()` details loop which is not atomic. This fix ignores any `404`s while retrieving category details.

I did not change the method signature as I consider this a bug fix and not a breaking API change.
I updated the doc strings to line wrap and be more consistent in this file.

Closes: #2710
Signed-off-by: Michael Gasch <mgasch@vmware.com>

## Type of change

Please mark options that are relevant:

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] 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?

Added a unit test simulating concurrent deletes while calling `GetCategories()`.

## 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
- [ ] 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


Patch:
diff --git a/sts/signer.go b/sts/signer.go
index 6ee0ffde..c6bf49d9 100644
--- a/sts/signer.go
+++ b/sts/signer.go
@@ -351,5 +351,5 @@ func isIPv6(s string) bool {
 	if ip == nil {
 		return false
 	}
-	return ip.To4() == nil
+	return len(ip) == net.IPv6len
 }
diff --git a/vapi/tags/categories.go b/vapi/tags/categories.go
index 3ad81820..64d0b66c 100644
--- a/vapi/tags/categories.go
+++ b/vapi/tags/categories.go
@@ -20,11 +20,13 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"strings"
 
 	"github.com/vmware/govmomi/vapi/internal"
 )
 
-// Category provides methods to create, read, update, delete, and enumerate categories.
+// Category provides methods to create, read, update, delete, and enumerate
+// categories.
 type Category struct {
 	ID              string   `json:"id,omitempty"`
 	Name            string   `json:"name,omitempty"`
@@ -92,7 +94,8 @@ func (c *Manager) CreateCategory(ctx context.Context, category *Category) (strin
 	return res, c.Do(ctx, url.Request(http.MethodPost, spec), &res)
 }
 
-// UpdateCategory can update one or more of the AssociableTypes, Cardinality, Description and Name fields.
+// UpdateCategory updates one or more of the AssociableTypes, Cardinality,
+// Description and Name fields.
 func (c *Manager) UpdateCategory(ctx context.Context, category *Category) error {
 	spec := struct {
 		Category Category `json:"update_spec"`
@@ -108,7 +111,7 @@ func (c *Manager) UpdateCategory(ctx context.Context, category *Category) error
 	return c.Do(ctx, url.Request(http.MethodPatch, spec), nil)
 }
 
-// DeleteCategory deletes an existing category.
+// DeleteCategory deletes a category.
 func (c *Manager) DeleteCategory(ctx context.Context, category *Category) error {
 	url := c.Resource(internal.CategoryPath).WithID(category.ID)
 	return c.Do(ctx, url.Request(http.MethodDelete), nil)
@@ -141,7 +144,7 @@ func (c *Manager) ListCategories(ctx context.Context) ([]string, error) {
 	return res, c.Do(ctx, url.Request(http.MethodGet), &res)
 }
 
-// GetCategories fetches an array of category information in the system.
+// GetCategories fetches a list of category information in the system.
 func (c *Manager) GetCategories(ctx context.Context) ([]Category, error) {
 	ids, err := c.ListCategories(ctx)
 	if err != nil {
@@ -152,11 +155,13 @@ func (c *Manager) GetCategories(ctx context.Context) ([]Category, error) {
 	for _, id := range ids {
 		category, err := c.GetCategory(ctx, id)
 		if err != nil {
-			return nil, fmt.Errorf("get category %s: %s", id, err)
+			if strings.Contains(err.Error(), http.StatusText(http.StatusNotFound)) {
+				continue // deleted since last fetch
+			}
+			return nil, fmt.Errorf("get category %s: %v", id, err)
 		}
-
 		categories = append(categories, *category)
-
 	}
+
 	return categories, nil
 }


Test patch:
diff --git a/sts/client_test.go b/sts/client_test.go
index 3b49b1ad..8196e50c 100644
--- a/sts/client_test.go
+++ b/sts/client_test.go
@@ -30,7 +30,6 @@ import (
 	"github.com/vmware/govmomi/session"
 	"github.com/vmware/govmomi/ssoadmin"
 	"github.com/vmware/govmomi/ssoadmin/types"
-	"github.com/vmware/govmomi/vapi/rest"
 	"github.com/vmware/govmomi/vim25"
 	"github.com/vmware/govmomi/vim25/methods"
 	"github.com/vmware/govmomi/vim25/soap"
@@ -161,12 +160,6 @@ func TestIssueHOK(t *testing.T) {
 	}
 
 	log.Printf("current time=%s", now)
-
-	rc := rest.NewClient(c)
-	err = rc.LoginByToken(rc.WithSigner(ctx, s))
-	if err != nil {
-		t.Fatal(err)
-	}
 }
 
 func TestIssueTokenByToken(t *testing.T) {
diff --git a/sts/signer_test.go b/sts/signer_test.go
index 6acacc51..f857b612 100644
--- a/sts/signer_test.go
+++ b/sts/signer_test.go
@@ -140,20 +140,3 @@ func TestSigner(t *testing.T) {
 		t.Fatal(err)
 	}
 }
-
-func TestIsIPv6(t *testing.T) {
-	tests := []struct {
-		ip string
-		v6 bool
-	}{
-		{"0:0:0:0:0:0:0:1", true},
-		{"10.0.0.42", false},
-	}
-
-	for _, test := range tests {
-		v6 := isIPv6(test.ip)
-		if v6 != test.v6 {
-			t.Errorf("%s: expected %t, got %t", test.ip, test.v6, v6)
-		}
-	}
-}
diff --git a/vapi/tags/categories_test.go b/vapi/tags/categories_test.go
new file mode 100644
index 00000000..66494856
--- /dev/null
+++ b/vapi/tags/categories_test.go
@@ -0,0 +1,87 @@
+package tags
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"strings"
+	"testing"
+
+	"github.com/vmware/govmomi/simulator"
+	"github.com/vmware/govmomi/vapi/rest"
+	"github.com/vmware/govmomi/vim25"
+)
+
+func TestManager_GetCategories(t *testing.T) {
+	simulator.Run(func(ctx context.Context, client *vim25.Client) error {
+		rc := rest.NewClient(client)
+
+		tr := testRoundTripper{
+			t:         t,
+			transport: rc.Client.Client.Transport,
+		}
+		rc.Client.Client.Transport = &tr
+
+		err := rc.Login(ctx, simulator.DefaultLogin)
+		if err != nil {
+			t.Fatalf("VAPI login: %v", err)
+		}
+
+		tm := NewManager(rc)
+
+		// categories to createCount and (concurrently) delete while retrieving categories
+		const (
+			createCount = 5
+			deleteCount = 2
+		)
+
+		var created []string
+		for i := 1; i <= createCount; i++ {
+			cat, err := tm.CreateCategory(ctx, &Category{
+				Name: fmt.Sprintf("testcat-%d", i),
+			})
+			if err != nil {
+				t.Fatalf("createCount category: %v", err)
+			}
+			created = append(created, cat)
+		}
+
+		for i := 0; i < deleteCount; i++ {
+			tr.deleted = append(tr.deleted, created[i])
+		}
+
+		got, err := tm.GetCategories(ctx)
+		if err != nil {
+			t.Fatalf("get categories: %v", err)
+		}
+
+		if len(got) != createCount-deleteCount {
+			t.Errorf("category count mismatch: got=%d, want=%d", len(got), createCount-deleteCount)
+		}
+
+		return nil
+	})
+}
+
+// testRoundTripper returns 404 for all categories in deleted
+type testRoundTripper struct {
+	t *testing.T
+
+	deleted   []string
+	transport http.RoundTripper
+}
+
+func (tr *testRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	for _, id := range tr.deleted {
+		if strings.Contains(req.URL.Path, id) {
+			return &http.Response{
+				StatusCode: http.StatusNotFound,
+				Status:     http.StatusText(http.StatusNotFound),
+				Body:       nil,
+				Request:    req.Clone(context.Background()),
+			}, nil
+		}
+	}
+
+	return tr.transport.RoundTrip(req)
+}