Skip to content

Commit

Permalink
Apply ownership for UC objects during creation (databricks#1338)
Browse files Browse the repository at this point in the history 
The current behaviour of Unity Catalog API is as follows:

- `databricks_catalog`, `databricks_metastore`, `databricks_external_location`, `databricks_schema`, `databricks_storage_credentials`, `databricks_table` have an owner field
- on Create, the new object’s owner field is set to the user performing the operation
- to ensure this field is set correctly in Terraform - we need to patch the resource after creating it

Changes applied:
* set owner during creation for UC securables
* updated docs
* refactored updates into a function factory
* added support for locations & credentials
* removed update APIs
* add unit test when cannot delete default schema
* add MarkNewResource to testing fixture
  • Loading branch information
@nkvuong
nkvuong committed Jun 3, 2022
1 parent e217111 commit c805bf2
Show file tree
Hide file tree
Showing 20 changed files with 542 additions and 100 deletions.
13 changes: 3 additions & 10 deletions catalog/resource_catalog.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,6 @@ func (a CatalogsAPI) getCatalog(name string) (ci CatalogInfo, err error) {
return
}

func (a CatalogsAPI) updateCatalog(ci CatalogInfo) error {
return a.client.Patch(a.context, "/unity-catalog/catalogs/"+ci.Name, ci)
}

func (a CatalogsAPI) deleteCatalog(name string) error {
// TODO: force_destroy attribute
return a.client.Delete(a.context, "/unity-catalog/catalogs/"+name+"?force=true", nil)
Expand All @@ -57,6 +53,7 @@ func ResourceCatalog() *schema.Resource {
func(m map[string]*schema.Schema) map[string]*schema.Schema {
return m
})
update := updateFunctionFactory("/unity-catalog/catalogs", []string{"owner", "comment", "properties"})
return common.Resource{
Schema: catalogSchema,
Create: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
Expand All @@ -69,7 +66,7 @@ func ResourceCatalog() *schema.Resource {
return fmt.Errorf("cannot remove new catalog default schema: %w", err)
}
d.SetId(ci.Name)
return nil
return update(ctx, d, c)
},
Read: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
ci, err := NewCatalogsAPI(ctx, c).getCatalog(d.Id())
Expand All @@ -78,11 +75,7 @@ func ResourceCatalog() *schema.Resource {
}
return common.StructToData(ci, catalogSchema, d)
},
Update: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
var ci CatalogInfo
common.DataToStructPointer(d, catalogSchema, &ci)
return NewCatalogsAPI(ctx, c).updateCatalog(ci)
},
Update: update,
Delete: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
return NewCatalogsAPI(ctx, c).deleteCatalog(d.Id())
},
Expand Down
140 changes: 140 additions & 0 deletions catalog/resource_catalog_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package catalog

import (
"fmt"
"testing"

"github.com/databrickslabs/terraform-provider-databricks/common"
"github.com/databrickslabs/terraform-provider-databricks/qa"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestCatalogCornerCases(t *testing.T) {
Expand Down Expand Up @@ -53,3 +57,139 @@ func TestCatalogCreateAlsoDeletesDefaultSchema(t *testing.T) {
`,
}.ApplyNoError(t)
}

func TestCatalogCreateWithOwnerAlsoDeletesDefaultSchema(t *testing.T) {
qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "POST",
Resource: "/api/2.0/unity-catalog/catalogs",
ExpectedRequest: CatalogInfo{
Name: "a",
Comment: "b",
Properties: map[string]string{
"c": "d",
},
Owner: "administrators",
},
Response: CatalogInfo{
Name: "a",
Comment: "b",
Properties: map[string]string{
"c": "d",
},
Owner: "testers",
},
},
{
Method: "DELETE",
Resource: "/api/2.0/unity-catalog/schemas/a.default",
},
{
Method: "PATCH",
Resource: "/api/2.0/unity-catalog/catalogs/a",
ExpectedRequest: map[string]interface{}{
"owner": "administrators",
},
},
{
Method: "GET",
Resource: "/api/2.0/unity-catalog/catalogs/a",
Response: CatalogInfo{
Name: "a",
Comment: "b",
Properties: map[string]string{
"c": "d",
},
MetastoreID: "e",
Owner: "administrators",
},
},
},
Resource: ResourceCatalog(),
Create: true,
HCL: `
name = "a"
comment = "b"
properties = {
c = "d"
}
owner = "administrators"
`,
}.ApplyNoError(t)
}

func TestCatalogCreateCannotDeleteDefaultSchema(t *testing.T) {
_, err := qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "POST",
Resource: "/api/2.0/unity-catalog/catalogs",
ExpectedRequest: CatalogInfo{
Name: "a",
Comment: "b",
Properties: map[string]string{
"c": "d",
},
},
},
{
Method: "DELETE",
Resource: "/api/2.0/unity-catalog/schemas/a.default",
Status: 400,
Response: common.APIErrorBody{
ScimDetail: "Something",
ScimStatus: "Else",
},
},
},
Resource: ResourceCatalog(),
Create: true,
HCL: `
name = "a"
comment = "b"
properties = {
c = "d"
}
`,
}.Apply(t)
require.Error(t, err, err)
assert.Equal(t, "cannot remove new catalog default schema: Something", fmt.Sprint(err))
}

func TestUpdateCatalog(t *testing.T) {
qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "PATCH",
Resource: "/api/2.0/unity-catalog/catalogs/a",
ExpectedRequest: map[string]interface{}{
"owner": "administrators",
},
},
{
Method: "GET",
Resource: "/api/2.0/unity-catalog/catalogs/a",
Response: CatalogInfo{
Name: "a",
MetastoreID: "d",
Comment: "c",
Owner: "administrators",
},
},
},
Resource: ResourceCatalog(),
Update: true,
ID: "a",
InstanceState: map[string]string{
"metastore_id": "d",
"name": "a",
"comment": "c",
},
HCL: `
name = "a"
comment = "c"
owner = "administrators"
`,
}.ApplyNoError(t)
}
21 changes: 4 additions & 17 deletions catalog/resource_external_location.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ func (a ExternalLocationsAPI) get(name string) (el ExternalLocationInfo, err err
return
}

func (a ExternalLocationsAPI) update(name string, el ExternalLocationInfo) error {
return a.client.Patch(a.context, "/unity-catalog/external-locations/"+url.PathEscape(name), el)
}

func (a ExternalLocationsAPI) delete(name string) error {
return a.client.Delete(a.context, "/unity-catalog/external-locations/"+url.PathEscape(name), nil)
}
Expand All @@ -49,17 +45,19 @@ func ResourceExternalLocation() *schema.Resource {
func(m map[string]*schema.Schema) map[string]*schema.Schema {
return m
})
update := updateFunctionFactory("/unity-catalog/external-locations", []string{"owner", "comment", "url", "credential_name"})
return common.Resource{
Schema: s,
Create: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
var el ExternalLocationInfo
common.DataToStructPointer(d, s, &el)
el.Owner = ""
err := NewExternalLocationsAPI(ctx, c).create(&el)
if err != nil {
return err
}
d.SetId(el.Name)
return nil
return update(ctx, d, c)
},
Read: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
el, err := NewExternalLocationsAPI(ctx, c).get(d.Id())
Expand All @@ -68,18 +66,7 @@ func ResourceExternalLocation() *schema.Resource {
}
return common.StructToData(el, s, d)
},
Update: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
var el ExternalLocationInfo
common.DataToStructPointer(d, s, &el)
return NewExternalLocationsAPI(ctx, c).update(d.Id(), ExternalLocationInfo{
Name: d.Id(),
URL: el.URL,
CredentialName: el.CredentialName,
SkipValidation: el.SkipValidation,
Comment: el.Comment,
Owner: el.Owner,
})
},
Update: update,
Delete: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
return NewExternalLocationsAPI(ctx, c).delete(d.Id())
},
Expand Down
80 changes: 80 additions & 0 deletions catalog/resource_external_location_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,83 @@ func TestCreateExternalLocation(t *testing.T) {
`,
}.ApplyNoError(t)
}

func TestCreateExternalLocationWithOwner(t *testing.T) {
qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "POST",
Resource: "/api/2.0/unity-catalog/external-locations",
ExpectedRequest: ExternalLocationInfo{
Name: "abc",
URL: "s3://foo/bar",
CredentialName: "bcd",
Comment: "def",
},
},
{
Method: "PATCH",
Resource: "/api/2.0/unity-catalog/external-locations/abc",
ExpectedRequest: map[string]interface{}{
"owner": "administrators",
},
},
{
Method: "GET",
Resource: "/api/2.0/unity-catalog/external-locations/abc",
Response: ExternalLocationInfo{
Owner: "administrators",
MetastoreID: "fgh",
},
},
},
Resource: ResourceExternalLocation(),
Create: true,
HCL: `
name = "abc"
url = "s3://foo/bar"
credential_name = "bcd"
owner = "administrators"
comment = "def"
`,
}.ApplyNoError(t)
}

func TestUpdateExternalLocation(t *testing.T) {
qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "PATCH",
Resource: "/api/2.0/unity-catalog/external-locations/abc",
ExpectedRequest: map[string]interface{}{
"credential_name": "bcd",
},
},
{
Method: "GET",
Resource: "/api/2.0/unity-catalog/external-locations/abc",
Response: ExternalLocationInfo{
Name: "abc",
URL: "s3://foo/bar",
CredentialName: "bcd",
Comment: "def",
},
},
},
Resource: ResourceExternalLocation(),
Update: true,
ID: "abc",
InstanceState: map[string]string{
"name": "abc",
"url": "s3://foo/bar",
"credential_name": "abc",
"comment": "def",
},
HCL: `
name = "abc"
url = "s3://foo/bar"
credential_name = "bcd"
comment = "def"
`,
}.ApplyNoError(t)
}
29 changes: 2 additions & 27 deletions catalog/resource_metastore.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,34 +87,9 @@ func ResourceMetastore() *schema.Resource {
}
return m
})
update := func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
// other fields to come later
updatable := []string{"owner", "name", "delta_sharing_enabled",
"delta_sharing_recipient_token_lifetime_in_seconds", "delta_sharing_organization_name"}
patch := map[string]interface{}{}
for _, field := range updatable {
old, new := d.GetChange(field)
if old == new {
continue
}
if field == "name" && old == "" {
continue
}
// delta sharing enabled and new is true must always be accompanied by a value for
// delta_sharing_recipient_token_lifetime_in_seconds
if field == "delta_sharing_enabled" && old != new && new == true &&
!d.HasChange("delta_sharing_recipient_token_lifetime_in_seconds") {
patch["delta_sharing_recipient_token_lifetime_in_seconds"] =
d.Get("delta_sharing_recipient_token_lifetime_in_seconds")
}
update := updateFunctionFactory("/unity-catalog/metastores", []string{"owner", "name", "delta_sharing_enabled",
"delta_sharing_recipient_token_lifetime_in_seconds", "delta_sharing_organization_name"})

patch[field] = new
}
if len(patch) == 0 {
return nil
}
return NewMetastoresAPI(ctx, c).updateMetastore(d.Id(), patch)
}
return common.Resource{
Schema: s,
Create: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
Expand Down
Loading

0 comments on commit c805bf2

Please sign in to comment.