Skip to content

Commit b6c91e5

Browse files
DidainiusDidainius
authored
V2.12.0 regression fix for org.GetCatalogByName and org.GetCatalogById (#389)
* go-vcloud-director does not find catalogs shared from other Orgs in methods org.GetCatalogByName and org.GetCatalogById This addon fixes that, adds some tests including a test with uses Org Admin user and adds a changelog note for 2.12.1 bugfix release.
1 parent 0c385c2 commit b6c91e5

File tree

6 files changed

+241
-22
lines changed

6 files changed

+241
-22
lines changed

‎CHANGELOG.md

+7
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,10 @@
1+
## 2.12.1 (5 July, 2021)
2+
3+
BUGS FIXED:
4+
* org.GetCatalogByName and org.GetCatalogById could not retrieve shared catalogs from different Orgs
5+
[#389](https://github.com/vmware/go-vcloud-director/pull/389)
6+
7+
18
## 2.12.0 (June 30, 2021)
29

310
* Added method `vdc.QueryEdgeGateway` [#364](https://github.com/vmware/go-vcloud-director/pull/364)

‎govcd/api_vcd_test.go

-1
Original file line numberDiff line numberDiff line change
@@ -1852,7 +1852,6 @@ func skipOpenApiEndpointTest(vcd *TestVCD, check *C, endpoint string) {
18521852
}
18531853

18541854
// newOrgUserConnection creates a new Org User and returns a connection to it
1855-
//lint:ignore U1000 For future usage - Allows writing tests that require multiple users
18561855
func newOrgUserConnection(adminOrg *AdminOrg, userName, password, href string, insecure bool) (*VCDClient, error) {
18571856
u, err := url.ParseRequestURI(href)
18581857
if err != nil {

‎govcd/catalog_test.go

+186
Original file line numberDiff line numberDiff line change
@@ -597,3 +597,189 @@ func (vcd *TestVCD) TestGetVappTemplateByHref(check *C) {
597597
check.Assert(vappTemplate.VAppTemplate.Type, Equals, types.MimeVAppTemplate)
598598
check.Assert(vappTemplate.VAppTemplate.Name, Equals, catalogItem.CatalogItem.Name)
599599
}
600+
601+
// Test_GetCatalogByNameSharedCatalog creates a separate Org and VDC just to create Catalog and share it with main Org
602+
// One should be able to find shared catalogs from different Organizations
603+
func (vcd *TestVCD) Test_GetCatalogByNameSharedCatalog(check *C) {
604+
fmt.Printf("Running: %s\n", check.TestName())
605+
606+
newOrg1, vdc, sharedCatalog := createSharedCatalogInNewOrg(vcd, check, check.TestName())
607+
608+
// Try to find the catalog inside Org which owns it - newOrg1
609+
catalogByName, err := newOrg1.GetCatalogByName(sharedCatalog.Catalog.Name, true)
610+
check.Assert(err, IsNil)
611+
check.Assert(catalogByName.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
612+
613+
// Try to find the catalog in another Org with which this catalog is shared (vcd.Org)
614+
sharedCatalogByName, err := vcd.org.GetCatalogByName(sharedCatalog.Catalog.Name, false)
615+
check.Assert(err, IsNil)
616+
check.Assert(sharedCatalogByName.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
617+
618+
cleanupCatalogOrgVdc(check, sharedCatalog, vdc, vcd, newOrg1)
619+
}
620+
621+
// Test_GetCatalogByIdSharedCatalog creates a separate Org and VDC just to create Catalog and share it with main Org
622+
// One should be able to find shared catalogs from different Organizations
623+
func (vcd *TestVCD) Test_GetCatalogByIdSharedCatalog(check *C) {
624+
fmt.Printf("Running: %s\n", check.TestName())
625+
626+
newOrg1, vdc, sharedCatalog := createSharedCatalogInNewOrg(vcd, check, check.TestName())
627+
628+
// Try to find the sharedCatalog inside Org which owns it - newOrg1
629+
catalogById, err := newOrg1.GetCatalogById(sharedCatalog.Catalog.ID, true)
630+
check.Assert(err, IsNil)
631+
check.Assert(catalogById.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
632+
633+
// Try to find the sharedCatalog in another Org with which this sharedCatalog is shared (vcd.Org)
634+
sharedCatalogById, err := vcd.org.GetCatalogById(sharedCatalog.Catalog.ID, false)
635+
check.Assert(err, IsNil)
636+
check.Assert(sharedCatalogById.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
637+
638+
cleanupCatalogOrgVdc(check, sharedCatalog, vdc, vcd, newOrg1)
639+
}
640+
641+
// Test_GetCatalogByNamePrefersLocal tests that local catalog (in the same Org) is prioritised against shared catalogs
642+
// in other Orgs. It does so by creating another Org with shared Catalog named just like the one in testing catalog
643+
func (vcd *TestVCD) Test_GetCatalogByNamePrefersLocal(check *C) {
644+
fmt.Printf("Running: %s\n", check.TestName())
645+
646+
// Create a catalog in new org with exactly the same name as in vcd.Org
647+
newOrg1, vdc, sharedCatalog := createSharedCatalogInNewOrg(vcd, check, vcd.config.VCD.Catalog.Name)
648+
649+
// Make sure that the Owner Org HREF is the local one for vcd.Org catalog named vcd.config.VCD.Catalog.Name
650+
catalogByNameInTestOrg, err := vcd.org.GetCatalogByName(vcd.config.VCD.Catalog.Name, true)
651+
check.Assert(err, IsNil)
652+
check.Assert(catalogByNameInTestOrg.parent.orgName(), Equals, vcd.org.Org.Name)
653+
654+
// Make sure that the Owner Org HREF is the local one for vcd.Org catalog named vcd.config.VCD.Catalog.Name
655+
catalogByNameInNewOrg, err := newOrg1.GetCatalogByName(vcd.config.VCD.Catalog.Name, true)
656+
check.Assert(err, IsNil)
657+
check.Assert(catalogByNameInNewOrg.parent.orgName(), Equals, newOrg1.Org.Name)
658+
659+
cleanupCatalogOrgVdc(check, sharedCatalog, vdc, vcd, newOrg1)
660+
}
661+
662+
// Test_GetCatalogByNameSharedCatalogOrgUser additionally tests GetOrgByName and GetOrgById using a custom created Org
663+
// Admin user. It tests the following cases:
664+
// * System user must be able to retrieve any catalog - shared or unshared from another Org
665+
// * Org Admin user must be able to retrieve catalog in his own Org
666+
// * Org Admin user must be able to retrieve shared catalog from another Org
667+
// * Org admin user must not be able to retrieve unshared catalog from another Org
668+
func (vcd *TestVCD) Test_GetCatalogByXSharedCatalogOrgUser(check *C) {
669+
fmt.Printf("Running: %s\n", check.TestName())
670+
newOrg1, vdc, sharedCatalog := createSharedCatalogInNewOrg(vcd, check, check.TestName())
671+
672+
// Create one more additional catalog which is not shared
673+
unsharedCatalog, err := newOrg1.CreateCatalog("unshared-catalog", check.TestName())
674+
check.Assert(err, IsNil)
675+
AddToCleanupList(unsharedCatalog.Catalog.Name, "catalog", newOrg1.Org.Name, check.TestName())
676+
677+
// Try to find the catalog inside Org which owns it - newOrg1
678+
catalogByName, err := newOrg1.GetCatalogByName(sharedCatalog.Catalog.Name, true)
679+
check.Assert(err, IsNil)
680+
check.Assert(catalogByName.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
681+
682+
// Try to find the catalog in another Org with which this catalog is shared (vcd.Org)
683+
sharedCatalogByName, err := vcd.org.GetCatalogByName(sharedCatalog.Catalog.Name, false)
684+
check.Assert(err, IsNil)
685+
check.Assert(sharedCatalogByName.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
686+
687+
// Try to find unshared catalog from another Org with System user
688+
systemUnsharedCatalogByName, err := vcd.org.GetCatalogByName(unsharedCatalog.Catalog.Name, true)
689+
check.Assert(err, IsNil)
690+
check.Assert(systemUnsharedCatalogByName.Catalog.ID, Equals, unsharedCatalog.Catalog.ID)
691+
692+
// Create an Org Admin user and test that it can find catalog as well
693+
adminOrg, err := vcd.client.GetAdminOrgByName(vcd.config.VCD.Org)
694+
check.Assert(err, IsNil)
695+
orgAdminClient, err := newOrgUserConnection(adminOrg, "test-user", "CHANGE-ME", vcd.config.Provider.Url, true)
696+
check.Assert(err, IsNil)
697+
orgAsOrgUser, err := orgAdminClient.GetOrgByName(vcd.config.VCD.Org)
698+
check.Assert(err, IsNil)
699+
700+
// Find a catalog in the same Org using Org Admin user
701+
orgAdminCatalogByNameSameOrg, err := orgAsOrgUser.GetCatalogByName(vcd.config.VCD.Catalog.Name, false)
702+
check.Assert(err, IsNil)
703+
check.Assert(orgAdminCatalogByNameSameOrg.Catalog.Name, Equals, vcd.config.VCD.Catalog.Name)
704+
705+
orgAdminCatalogByIdSameOrg, err := orgAsOrgUser.GetCatalogById(orgAdminCatalogByNameSameOrg.Catalog.ID, false)
706+
check.Assert(err, IsNil)
707+
check.Assert(orgAdminCatalogByIdSameOrg.Catalog.Name, Equals, orgAdminCatalogByNameSameOrg.Catalog.Name)
708+
check.Assert(orgAdminCatalogByIdSameOrg.Catalog.ID, Equals, orgAdminCatalogByNameSameOrg.Catalog.ID)
709+
710+
// Find a shared catalog from another Org using Org Admin user
711+
orgAdminCatalogByName, err := orgAsOrgUser.GetCatalogByName(sharedCatalog.Catalog.Name, false)
712+
check.Assert(err, IsNil)
713+
check.Assert(orgAdminCatalogByName.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
714+
check.Assert(orgAdminCatalogByName.Catalog.ID, Equals, sharedCatalog.Catalog.ID)
715+
716+
orgAdminCatalogById, err := orgAsOrgUser.GetCatalogById(sharedCatalog.Catalog.ID, false)
717+
check.Assert(err, IsNil)
718+
check.Assert(orgAdminCatalogById.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
719+
check.Assert(orgAdminCatalogById.Catalog.ID, Equals, sharedCatalog.Catalog.ID)
720+
721+
// Try to find unshared catalog from another Org with Org admin user and expect an ErrorEntityNotFound
722+
_, err = orgAsOrgUser.GetCatalogByName(unsharedCatalog.Catalog.Name, true)
723+
check.Assert(ContainsNotFound(err), Equals, true)
724+
725+
_, err = orgAsOrgUser.GetCatalogById(unsharedCatalog.Catalog.ID, true)
726+
check.Assert(ContainsNotFound(err), Equals, true)
727+
728+
// Cleanup
729+
err = unsharedCatalog.Delete(true, true)
730+
check.Assert(err, IsNil)
731+
732+
cleanupCatalogOrgVdc(check, sharedCatalog, vdc, vcd, newOrg1)
733+
}
734+
735+
func createSharedCatalogInNewOrg(vcd *TestVCD, check *C, newCatalogName string) (*Org, *Vdc, Catalog) {
736+
newOrgName1 := spawnTestOrg(vcd, check, "org")
737+
738+
newOrg1, err := vcd.client.GetOrgByName(newOrgName1)
739+
check.Assert(err, IsNil)
740+
741+
// Spawn a VDC inside newly created Org so that there is storage to create new catalog
742+
vdc := spawnTestVdc(vcd, check, newOrgName1)
743+
744+
catalog, err := newOrg1.CreateCatalog(newCatalogName, "Catalog for testing")
745+
check.Assert(err, IsNil)
746+
AddToCleanupList(newCatalogName, "catalog", newOrgName1, check.TestName())
747+
748+
// Share new Catalog in newOrgName1 with default test Org vcd.Org
749+
readOnly := "ReadOnly"
750+
accessControl := &types.ControlAccessParams{
751+
IsSharedToEveryone: false,
752+
EveryoneAccessLevel: &readOnly,
753+
AccessSettings: &types.AccessSettingList{
754+
AccessSetting: []*types.AccessSetting{&types.AccessSetting{
755+
Subject: &types.LocalSubject{
756+
HREF: vcd.org.Org.HREF,
757+
Name: vcd.org.Org.Name,
758+
Type: types.MimeOrg,
759+
},
760+
AccessLevel: "ReadOnly",
761+
}},
762+
},
763+
}
764+
err = catalog.SetAccessControl(accessControl, false)
765+
check.Assert(err, IsNil)
766+
767+
return newOrg1, vdc, catalog
768+
}
769+
770+
func cleanupCatalogOrgVdc(check *C, sharedCatalog Catalog, vdc *Vdc, vcd *TestVCD, newOrg1 *Org) {
771+
// Cleanup catalog, vdc and org
772+
err := sharedCatalog.Delete(true, true)
773+
check.Assert(err, IsNil)
774+
775+
// There are cases where it just takes a a few seconds after catalog deletion when one can delete VDC
776+
time.Sleep(2 * time.Second)
777+
778+
err = vdc.DeleteWait(true, true)
779+
check.Assert(err, IsNil)
780+
781+
adminOrg, err := vcd.client.GetAdminOrgByName(newOrg1.Org.Name)
782+
check.Assert(err, IsNil)
783+
err = adminOrg.Delete(true, true)
784+
check.Assert(err, IsNil)
785+
}

‎govcd/common_test.go

+14
Original file line numberDiff line numberDiff line change
@@ -793,6 +793,20 @@ func spawnTestVdc(vcd *TestVCD, check *C, adminOrgName string) *Vdc {
793793
return vdc
794794
}
795795

796+
// spawnTestOrg spawns an Org to be used in tests
797+
func spawnTestOrg(vcd *TestVCD, check *C, nameSuffix string) string {
798+
newOrg, err := vcd.client.GetAdminOrgByName(vcd.config.VCD.Org)
799+
check.Assert(err, IsNil)
800+
newOrgName := check.TestName() + "-" + nameSuffix
801+
task, err := CreateOrg(vcd.client, newOrgName, newOrgName, newOrgName, newOrg.AdminOrg.OrgSettings, true)
802+
check.Assert(err, IsNil)
803+
err = task.WaitTaskCompletion()
804+
check.Assert(err, IsNil)
805+
AddToCleanupList(newOrgName, "org", "", check.TestName())
806+
807+
return newOrgName
808+
}
809+
796810
func getVdcProviderVdcHref(vcd *TestVCD, check *C) string {
797811
results, err := vcd.client.QueryWithNotEncodedParams(nil, map[string]string{
798812
"type": "providerVdc",

‎govcd/org.go

+34-7
Original file line numberDiff line numberDiff line change
@@ -415,9 +415,11 @@ func (org *Org) queryOrgVdcById(vdcId string) (*types.QueryResultOrgVdcRecordTyp
415415
// queryCatalogByName returns a single CatalogRecord
416416
func (org *Org) queryCatalogByName(catalogName string) (*types.CatalogRecord, error) {
417417
filterMap := map[string]string{
418-
"org": org.Org.HREF, // Org ID is not allowed for non System
419-
"orgName": org.Org.Name,
420-
"name": catalogName,
418+
// Not injecting `org` or `orgName` here because shared catalogs may also appear here and they would have different
419+
// parent Org
420+
// "org": org.Org.HREF,
421+
// "orgName": org.Org.Name,
422+
"name": catalogName,
421423
}
422424
allCatalogs, err := queryCatalogList(org.client, filterMap)
423425
if err != nil {
@@ -428,19 +430,44 @@ func (org *Org) queryCatalogByName(catalogName string) (*types.CatalogRecord, er
428430
return nil, ErrorEntityNotFound
429431
}
430432

433+
// To conform with this API standard it would be best to return an error if more than 1 item is found, but because
434+
// previous method of getting Catalog by Name returned the first result we are doing the same here
435+
// if len(allCatalogs) > 1 {
436+
// return nil, fmt.Errorf("found more than 1 Catalog with Name '%s'", catalogName)
437+
// }
438+
439+
var localCatalog *types.CatalogRecord
440+
// if multiple results are found - return the one defined in `org` (local)
431441
if len(allCatalogs) > 1 {
432-
return nil, fmt.Errorf("found more than 1 VDC with Name '%s'", catalogName)
442+
util.Logger.Printf("[DEBUG] org.queryCatalogByName found %d Catalogs by name '%s'", len(allCatalogs), catalogName)
443+
for _, catalog := range allCatalogs {
444+
util.Logger.Printf("[DEBUG] org.queryCatalogByName found a Catalog by name '%s' in Org '%s'", catalogName, catalog.OrgName)
445+
if catalog.OrgName == org.Org.Name {
446+
util.Logger.Printf("[DEBUG] org.queryCatalogByName Catalog '%s' is local for Org '%s'. Prioritising it",
447+
catalogName, org.Org.Name)
448+
// Not interrupting the loop here to still dump all results to logs
449+
localCatalog = catalog
450+
}
451+
}
433452
}
434453

454+
// local catalog was found - return it
455+
if localCatalog != nil {
456+
return localCatalog, nil
457+
}
458+
459+
// If only one catalog is found or multiple catalogs with no local ones - return the first one
435460
return allCatalogs[0], nil
436461
}
437462

438463
// queryCatalogById returns a single QueryResultOrgVdcRecordType
439464
func (org *Org) queryCatalogById(catalogId string) (*types.CatalogRecord, error) {
440465
filterMap := map[string]string{
441-
"org": org.Org.HREF,
442-
"orgName": org.Org.Name,
443-
"id": catalogId,
466+
// Not injecting `org` or `orgName` here because shared catalogs may also appear here and they would have different
467+
// parent Org
468+
// "org": org.Org.HREF,
469+
// "orgName": org.Org.Name,
470+
"id": catalogId,
444471
}
445472
allCatalogs, err := queryCatalogList(org.client, filterMap)
446473

‎govcd/org_test.go

-14
Original file line numberDiff line numberDiff line change
@@ -1107,17 +1107,3 @@ func validateQueryOrgVdcResults(vcd *TestVCD, check *C, name, orgName string, ex
11071107
fmt.Println()
11081108
}
11091109
}
1110-
1111-
// spawnTestOrg spawns an Org to be used in tests
1112-
func spawnTestOrg(vcd *TestVCD, check *C, nameSuffix string) string {
1113-
newOrg, err := vcd.client.GetAdminOrgByName(vcd.config.VCD.Org)
1114-
check.Assert(err, IsNil)
1115-
newOrgName := check.TestName() + "-" + nameSuffix
1116-
task, err := CreateOrg(vcd.client, newOrgName, newOrgName, newOrgName, newOrg.AdminOrg.OrgSettings, true)
1117-
check.Assert(err, IsNil)
1118-
err = task.WaitTaskCompletion()
1119-
check.Assert(err, IsNil)
1120-
AddToCleanupList(newOrgName, "org", "", check.TestName())
1121-
1122-
return newOrgName
1123-
}

0 commit comments

Comments
 (0)