diff --git a/lib/apiservers/service/restapi/handlers/vch_create.go b/lib/apiservers/service/restapi/handlers/vch_create.go index 21ef8b9022..3baec638be 100644 --- a/lib/apiservers/service/restapi/handlers/vch_create.go +++ b/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 { diff --git a/lib/apiservers/service/restapi/handlers/vch_create_test.go b/lib/apiservers/service/restapi/handlers/vch_create_test.go index 8bff57b62b..d2e06e5fde 100644 --- a/lib/apiservers/service/restapi/handlers/vch_create_test.go +++ b/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) diff --git a/tests/test-cases/Group23-VIC-Machine-Service/23-03-VCH-Create.robot b/tests/test-cases/Group23-VIC-Machine-Service/23-03-VCH-Create.robot index 5328efdd61..1d83f46887 100644 --- a/tests/test-cases/Group23-VIC-Machine-Service/23-03-VCH-Create.robot +++ b/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}}}'