Support ID-based selection of compute resources via the API

lib/apiservers/service/restapi/handlers/vch_create.go

@@ -178,13 +178,18 @@ func buildCreate(op trace.Operation, d *data.Data, finder finder, vch *models.VC
 				c.ResourceLimits.VCHMemoryShares = fromShares(vch.Compute.Memory.Shares)
 			}
 
-			resourcePath, err := fromManagedObject(op, finder, "ResourcePool", vch.Compute.Resource) // TODO (#6711): Do we need to handle clusters differently?
+			resourcePath, resourceType, err := fromManagedObject(op, finder, vch.Compute.Resource, "ResourcePool", "ComputeResource", "ClusterComputeResource", "HostSystem")
 			if err != nil {
 				return nil, util.NewError(http.StatusBadRequest, fmt.Sprintf("Error finding resource pool: %s", err))
 			}
 			if resourcePath == "" {
 				return nil, util.NewError(http.StatusBadRequest, "Resource pool must be specified (by name or id)")
 			}
+			if resourceType == "HostSystem" {
+				// When looking up a stand-alone host, the returned path will be like "/dc1/host/192.0.2.1/192.0.2.1".
+				// This is the correct path for the host, but we actually want the path for the host's "resource pool".
+				resourcePath = path.Dir(resourcePath)
+			}
 			c.ComputeResourcePath = resourcePath
 
 			if vch.Compute.Affinity != nil {
@@ -194,7 +199,7 @@ func buildCreate(op trace.Operation, d *data.Data, finder finder, vch *models.VC
 
 		if vch.Network != nil {
 			if vch.Network.Bridge != nil {
-				path, err := fromManagedObject(op, finder, "Network", vch.Network.Bridge.PortGroup)
+				path, _, err := fromManagedObject(op, finder, vch.Network.Bridge.PortGroup, "Network")
 				if err != nil {
 					return nil, util.NewError(http.StatusBadRequest, fmt.Sprintf("Error finding bridge network: %s", err))
 				}
@@ -210,7 +215,7 @@ func buildCreate(op trace.Operation, d *data.Data, finder finder, vch *models.VC
 			}
 
 			if vch.Network.Client != nil {
-				path, err := fromManagedObject(op, finder, "Network", vch.Network.Client.PortGroup)
+				path, _, err := fromManagedObject(op, finder, vch.Network.Client.PortGroup, "Network")
 				if err != nil {
 					return nil, util.NewError(http.StatusBadRequest, fmt.Sprintf("Error finding client network portgroup: %s", err))
 				}
@@ -227,7 +232,7 @@ func buildCreate(op trace.Operation, d *data.Data, finder finder, vch *models.VC
 			}
 
 			if vch.Network.Management != nil {
-				path, err := fromManagedObject(op, finder, "Network", vch.Network.Management.PortGroup)
+				path, _, err := fromManagedObject(op, finder, vch.Network.Management.PortGroup, "Network")
 				if err != nil {
 					return nil, util.NewError(http.StatusBadRequest, fmt.Sprintf("Error finding management network portgroup: %s", err))
 				}
@@ -244,7 +249,7 @@ func buildCreate(op trace.Operation, d *data.Data, finder finder, vch *models.VC
 			}
 
 			if vch.Network.Public != nil {
-				path, err := fromManagedObject(op, finder, "Network", vch.Network.Public.PortGroup)
+				path, _, err := fromManagedObject(op, finder, vch.Network.Public.PortGroup, "Network")
 				if err != nil {
 					return nil, util.NewError(http.StatusBadRequest, fmt.Sprintf("Error finding public network portgroup: %s", err))
 				}
@@ -280,7 +285,7 @@ func buildCreate(op trace.Operation, d *data.Data, finder finder, vch *models.VC
 				for _, cnetwork := range vch.Network.Container {
 					alias := cnetwork.Alias
 
-					path, err := fromManagedObject(op, finder, "Network", cnetwork.PortGroup)
+					path, _, err := fromManagedObject(op, finder, cnetwork.PortGroup, "Network")
 					if err != nil {
 						return nil, util.NewError(http.StatusBadRequest, fmt.Sprintf("Error finding portgroup for container network %s: %s", alias, err))
 					}
@@ -484,23 +489,29 @@ func handleCreate(op trace.Operation, c *create.Create, validator *validate.Vali
 	return nil, nil
 }
 
-func fromManagedObject(op trace.Operation, finder finder, t string, m *models.ManagedObject) (string, error) {
+// fromManagedObject returns a valid name/path for the supplied object and its type, if known.
+func fromManagedObject(op trace.Operation, finder finder, m *models.ManagedObject, ts ...string) (string, string, error) {
 	if m == nil {
-		return "", nil
+		return "", "", nil
 	}
 
 	if m.ID != "" {
-		managedObjectReference := types.ManagedObjectReference{Type: t, Value: m.ID}
-		element, err := finder.Element(op, managedObjectReference)
-
-		if err != nil {
-			return "", err
+		for _, t := range ts {
+			managedObjectReference := types.ManagedObjectReference{Type: t, Value: m.ID}
+			element, err := finder.Element(op, managedObjectReference)
+
+			if err == nil && element != nil {
+				return element.Path, t, nil
+			} else if err != nil {
+				// Ideally, we would continue only on *find.NotFoundError, but it is not reliably returned.
+				continue
+			}
 		}
 
-		return element.Path, nil
+		return "", "", fmt.Errorf("Unable to locate %q as any of %s", m.ID, ts)
 	}
 
-	return m.Name, nil
+	return m.Name, "", nil
 }
 
 func fromCIDR(m *models.CIDR) string {

lib/apiservers/service/restapi/handlers/vch_create_test.go

@@ -24,8 +24,10 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
 	cli "gopkg.in/urfave/cli.v1"
 
+	"github.com/vmware/govmomi/find"
 	"github.com/vmware/govmomi/list"
 	"github.com/vmware/govmomi/vim25/types"
 	"github.com/vmware/vic/cmd/vic-machine/common"
@@ -36,48 +38,135 @@ import (
 )
 
 type mockFinder struct {
+	mock.Mock
+
 	path string
 }
 
-func (mf mockFinder) Element(ctx context.Context, t types.ManagedObjectReference) (*list.Element, error) {
-	return &list.Element{
-		Path: t.Value,
-	}, nil
+func (mf *mockFinder) Element(ctx context.Context, t types.ManagedObjectReference) (*list.Element, error) {
+	args := mf.Called(ctx, t)
+
+	if p := args.Get(0); p != nil {
+		return &list.Element{
+			Path: p.(string),
+		}, args.Error(1)
+	}
+
+	return nil, args.Error(1)
 }
 
 func TestFromManagedObject(t *testing.T) {
 	op := trace.NewOperation(context.Background(), "TestFromManagedObject")
 	var m *models.ManagedObject
 
+	expectedType := "t"
+
 	expected := ""
-	actual, err := fromManagedObject(op, nil, "t", m)
+	actual, _, err := fromManagedObject(op, nil, m, expectedType)
 	assert.NoError(t, err, "Expected nil error, got %#v", err)
 	assert.Equal(t, expected, actual)
 
+	name := "testManagedObject"
+	path := "/folder/" + name
+
 	m = &models.ManagedObject{
-		Name: "testManagedObject",
+		Name: name,
 	}
 
-	mf := mockFinder{}
+	mf := &mockFinder{}
+	mf.On("Element", op, mock.AnythingOfType("types.ManagedObjectReference")).Return(path, nil)
 
-	expected = m.Name
-	actual, err = fromManagedObject(op, mf, "t", m)
+	expected = name
+	actual, _, err = fromManagedObject(op, mf, m, expectedType)
 	assert.NoError(t, err, "Expected nil error, got %#v", err)
 	assert.Equal(t, expected, actual)
 
 	m.ID = "testID"
 
-	expected = m.ID
-	actual, err = fromManagedObject(op, mf, "t", m)
+	expected = path
+	actual, actualType, err := fromManagedObject(op, mf, m, expectedType)
 	assert.NoError(t, err, "Expected nil error, got %#v", err)
 	assert.Equal(t, expected, actual)
+	assert.Equal(t, expectedType, actualType)
 
 	m.Name = ""
 
-	expected = m.ID
-	actual, err = fromManagedObject(op, mf, "t", m)
+	expected = path
+	actual, actualType, err = fromManagedObject(op, mf, m, expectedType)
 	assert.NoError(t, err, "Expected nil error, got %#v", err)
 	assert.Equal(t, expected, actual)
+	assert.Equal(t, expectedType, actualType)
+}
+
+func TestFromManagedObject_Negative(t *testing.T) {
+	op := trace.NewOperation(context.Background(), "TestFromManagedObject")
+
+	m := &models.ManagedObject{
+		ID: "testID",
+	}
+
+	mf := &mockFinder{}
+	mf.On("Element", op, mock.AnythingOfType("types.ManagedObjectReference")).Return(nil, nil)
+
+	expected := ""
+	actual, _, err := fromManagedObject(op, mf, m, "t")
+	assert.Error(t, err, "Expected error when no resource found")
+	assert.Equal(t, expected, actual)
+
+	expected = ""
+	actual, _, err = fromManagedObject(op, mf, m, "t", "u", "v")
+	assert.Error(t, err, "Expected error when no resource found")
+	assert.Equal(t, expected, actual)
+}
+
+func TestFromManagedObject_Fallback(t *testing.T) {
+	op := trace.NewOperation(context.Background(), "TestFromManagedObject")
+
+	m := &models.ManagedObject{
+		ID: "testID",
+	}
+
+	mf := &mockFinder{}
+	mf.On("Element", op, mock.MatchedBy(func(t types.ManagedObjectReference) bool { return t.Type == "t" })).Return(nil, &find.NotFoundError{})
+	mf.On("Element", op, mock.MatchedBy(func(t types.ManagedObjectReference) bool { return t.Type == "u" })).Return(nil, nil)
+	mf.On("Element", op, mock.MatchedBy(func(t types.ManagedObjectReference) bool { return t.Type == "v" })).Return("Result", nil)
+	mf.On("Element", op, mock.MatchedBy(func(t types.ManagedObjectReference) bool { return t.Type == "e" })).Return(nil, fmt.Errorf("Expected"))
+
+	expected := ""
+	actual, actualType, err := fromManagedObject(op, mf, m, "t")
+	assert.Error(t, err, "Expected error when NotFoundError encountered")
+	assert.Equal(t, expected, actual)
+	assert.Equal(t, "", actualType)
+
+	expected = ""
+	actual, actualType, err = fromManagedObject(op, mf, m, "t", "u")
+	assert.Error(t, err, "Expected error when no resource found")
+	assert.Equal(t, expected, actual)
+	assert.Equal(t, "", actualType)
+
+	expected = "Result"
+	actual, actualType, err = fromManagedObject(op, mf, m, "t", "u", "v")
+	assert.NoError(t, err, "Did not expect error when third type returns valid result, but got %#v", err)
+	assert.Equal(t, expected, actual)
+	assert.Equal(t, "v", actualType)
+
+	expected = "Result"
+	actual, actualType, err = fromManagedObject(op, mf, m, "t", "v")
+	assert.NoError(t, err, "Did not expect error when second type returns valid result, but got %#v", err)
+	assert.Equal(t, expected, actual)
+	assert.Equal(t, "v", actualType)
+
+	expected = "Result"
+	actual, actualType, err = fromManagedObject(op, mf, m, "u", "v")
+	assert.NoError(t, err, "Did not expect error when second type returns valid result, but got %#v", err)
+	assert.Equal(t, expected, actual)
+	assert.Equal(t, "v", actualType)
+
+	expected = "Result"
+	actual, actualType, err = fromManagedObject(op, mf, m, "u", "e", "v")
+	assert.NoError(t, err, "Did not expect error when third type returns valid result, but got %#v", err)
+	assert.Equal(t, expected, actual)
+	assert.Equal(t, "v", actualType)
 }
 
 func TestFromCIDR(t *testing.T) {
@@ -132,13 +221,11 @@ func TestCreateVCH(t *testing.T) {
 			Bridge: &models.VCHNetworkBridge{
 				IPRange: "17.16.0.0/12",
 				PortGroup: &models.ManagedObject{
-					ID:   "bridge", // required for mocked finder to work
 					Name: "bridge",
 				},
 			},
 			Public: &models.Network{
 				PortGroup: &models.ManagedObject{
-					ID:   "public", // required for mock finder to work
 					Name: "public",
 				},
 			},
@@ -197,7 +284,8 @@ func TestCreateVCH(t *testing.T) {
 	assert.NoError(t, err, "Error while processing params: %s", err)
 	op.Infof("ca EnvFile: %s", ca.Certs.EnvFile)
 
-	mf := mockFinder{}
+	mf := &mockFinder{}
+	mf.On("Element", op, mock.AnythingOfType("types.ManagedObjectReference")).Return(nil, nil)
 
 	cb, err := buildCreate(op, data, mf, vch)
 	assert.NoError(t, err, "Error while processing params: %s", err)

tests/test-cases/Group23-VIC-Machine-Service/23-03-VCH-Create.robot

@@ -91,6 +91,18 @@ Extract Docker IP And Port
     Set Test Variable    ${docker_host}
 
 
+Get Cluster ID
+    [Arguments]    ${name}=%{TEST_RESOURCE}
+    ${rc}    ${output}=    Run And Return Rc And Output    govc ls --json=true "${name}" | jq -r '.elements[] | select(.Path | endswith("/Resources")).Object.Parent.Value'
+    Should Be Equal As Integers    ${rc}    0
+
+    ${rc2}    ${output2}=    Run Keyword If     '${output}' == '${EMPTY}'    Run And Return Rc And Output    govc ls --json=true "*/${name}" | jq -r '.elements[] | select(.Path | endswith("/Resources")).Object.Parent.Value'
+                             Run Keyword If     '${output}' == '${EMPTY}'    Should Be Equal As Integers    ${rc2}    0
+    ${return}=               Set Variable If    '${output}' == '${EMPTY}'    ${output2}    ${output}
+
+    [Return]    ${return}
+
+
 *** Test Cases ***
 Create minimal VCH
     Create VCH    '{"name":"%{VCH-NAME}-api-test-minimal","compute":{"resource":{"name":"%{TEST_RESOURCE}"}},"storage":{"image_stores":["ds://%{TEST_DATASTORE}"]},"network":{"bridge":{"ip_range":"172.16.0.0/12","port_group":{"name":"%{BRIDGE_NETWORK}"}},"public":{"port_group":{"name":"${PUBLIC_NETWORK}"}}},"auth":{"server":{"generate":{"cname":"vch.example.com","organization":["VMware, Inc."],"size":{"value":2048,"units":"bits"}}},"client":{"no_tls_verify": true}}}'
@@ -128,6 +140,22 @@ Create minimal VCH
     [Teardown]    Run Secret VIC Machine Delete Command    %{VCH-NAME}-api-test-minimal
 
 
+Create minimal VCH using compute resource ID
+    ${cluster}=    Get Cluster ID
+
+    Create VCH    '{"name":"%{VCH-NAME}-api-test-cr-id","compute":{"resource":{"id":"${cluster}"}},"storage":{"image_stores":["ds://%{TEST_DATASTORE}"]},"network":{"bridge":{"ip_range":"172.16.0.0/12","port_group":{"name":"%{BRIDGE_NETWORK}"}},"public":{"port_group":{"name":"${PUBLIC_NETWORK}"}}},"auth":{"server":{"generate":{"cname":"vch.example.com","organization":["VMware, Inc."],"size":{"value":2048,"units":"bits"}}},"client":{"no_tls_verify": true}}}'
+
+    Verify Return Code
+    Verify Status Created
+
+
+    Get VCH %{VCH-NAME}-api-test-cr-id
+
+    Property Should Be Equal        .name                                %{VCH-NAME}-api-test-cr-id
+
+    [Teardown]    Run Secret VIC Machine Delete Command    %{VCH-NAME}-api-test-cr-id
+
+
 Create minimal VCH within datacenter
     Create VCH Within Datacenter    '{"name":"%{VCH-NAME}-api-test-dc","compute":{"resource":{"name":"%{TEST_RESOURCE}"}},"storage":{"image_stores":["ds://%{TEST_DATASTORE}"]},"network":{"bridge":{"ip_range":"172.16.0.0/12","port_group":{"name":"%{BRIDGE_NETWORK}"}},"public":{"port_group":{"name":"${PUBLIC_NETWORK}"}}},"auth":{"server":{"generate":{"cname":"vch.example.com","organization":["VMware, Inc."],"size":{"value":2048,"units":"bits"}}},"client":{"no_tls_verify": true}}}'