Add databricks_connection resource to support Lakehouse Federation

[nkvuong]

Changes

Add support for Lakehouse Federation

• Add a new resource databricks_connection that represent Unity Catalog connections
• Extend databricks_catalog to support foreign catalogs

Closes #2575

Tests

• make test run locally
• relevant change in docs/ folder
• covered with integration tests in internal/acceptance
• relevant acceptance tests are passing
• using Go SDK

[alexott]

looks good, minor comments that we can address later

[alexott]

I think that we have multiple places like this, so it makes sense to move this to the common package

[andrewli81]

This needs to be a list of options:

"options":[
  {"key":"host","value":"mysqk.fakedb.com"},
  {"key":"port","value":"1234"},
  {"key":"user","value":"user123"},
  {"key":"password","value":"password123"}
]

[andrewli81]

Right now it is not a list:

"options_kvpairs": {
   "host": "test.com"
}

[alexott]

Is it really expected that it will be repeated keys? Other APIs are accepting objects as well, in addition to key/value pairs...

[andrewli81]

I believe the api docs are wrong, this is something we need to correct. There's no difference between the connection api code and other apis.

[andrewli81]

cc @Doug-Luce - I am not sure if the open api spec actually works - the spec mentions the options in a KV pair way:

"options_kvpairs": {
   "host": "test.com",
   "port": "1234",
   "user": "user123",
   "password": "password123"
}

but I always know this works:

"options":[
  {"key":"host","value":"mysqk.fakedb.com"},
  {"key":"port","value":"1234"},
  {"key":"user","value":"user123"},
  {"key":"password","value":"password123"}
]

It could be possible that we have an implementation deficiency of the connection API that caused us to not be able to take the kv style of input, but I think most likely it's a bug in our API doc, which we should fix.

[mgyucht]

I queried the REST API and saw the following response:

<     {
<       "connection_id": "<REDACTED>",
<       "connection_type": "POSTGRESQL",
<       "created_at": 1681945142627,
<       "created_by": "<REDACTED>",
<       "credential_type": "USERNAME_PASSWORD",
<       "full_name": "<REDACTED>",
<       "metastore_id": "<REDACTED>",
<       "name": "<REDACTED>",
<       "options": {
<         "host": "<REDACTED>,
<         "port": "5432"
<       },
<       "owner": "<REDACTED>",
<       "provisioning_info": {
<         "state": "ACTIVE"
<       },
<       "read_only": true,
<       "securable_kind": "CONNECTION_POSTGRESQL",
<       "securable_type": "CONNECTION",
<       "updated_at": 1681945142627,
<       "updated_by": "<REDACTED>",
<       "url": "jdbc://<REDACTED>:5432/"
<     }

Options is named options and is a dictionary of strings. This response had no properties, but that field is modeled identically internally.

I'll file a ticket to correct the OpenAPI specification. In the meantime, it seems like we should model this as a map[string]string.

[Doug-Luce]

I'm updating the OpenAPI spec to fix the naming of options as well as to reflect that properties_kvpairs is not a part of the response. I'll also add a couple of missing fields.

For creating the connection, options are specified as map[string][string].
This is valid:

"options_kvpairs": {
   "host": "test.com",
   "port": "1234",
   "user": "user123",
   "password": "password123"
}

[mgyucht]

I think it should still be named options, as that is the name of the nested field inside after inlining?

[Doug-Luce]

Yes, it would be options and not options_kvpairs. Copy-pasted from Andrew's post, but this is the body I used to create the connection.

{
    "name": "openapi_connection_spec_test_2",
    "connection_type": "MYSQL",
    "options": {
        "host": "test.com",
        "port": "5432",
        "user": "dougluce",
        "password": "redacted"
    },
    "properties": {
        "some_prop": "test1",
        "some_prop2": "test2"
    }
}

[Doug-Luce]

PR fixing this here: https://github.com/databricks/universe/pull/363345

[andrewli81]

LGTM, thanks so much for adding this, one comment on why the integration test is failing.

This is an example good json input:

curl -vvv -H "Authorization: Bearer xxx" -H "Content-Type: application/json" -X POST https://dbc-01eafff1-4ea3.dev.databricks.com/api/2.0/unity-catalog/connections -d '{"name":"test_mysql","connection_type":"MYSQL","options":[{"key":"host","value":"mysqk.fakedb.com"},{"key":"port","value":"1234"},{"key":"user","value":"user123"},{"key":"password","value":"password123"}]}'

[mgyucht]

First batch of comments. Thanks for contributing this!

[mgyucht]

We need better utilities for converting Go SDK structs to TF schemas so we don't need to inline stuff like this.

[mgyucht]

micronit: Can you move this comment up one line?

[mgyucht]

Are connection names globally unique?

[nkvuong]

connection names must be unique within a metastore

[mgyucht]

What happens if a user defines two connections with the same name in two different metastores? Should the name be combined with the metastore ID to make a universally unique identifier?

[andrewli81]

Yeah if you want global uniqueness we should add metastore id into the scope.

[mgyucht]

Please fix the OpenAPI spec, as we (and customers!) depend heavily on its accuracy to be able to use your team's API.

BTW: do we really want this to be a repeated list of KV pairs? We should be critical about these interfaces and think from the customer's perspective, aiming to do what is natural and intuitive where possible, rather than force all customers to adapt to an unusual pattern because there is some small technical issue. In the worst case, you should be able to define your proto using the Any type and deserialize yourself (scalapb docs here: https://scalapb.github.io/api/com/google/protobuf/any/Any.html).

[mgyucht]

Remove this

[mgyucht]

Couple questions, otherwise seems mostly alright. Main question is what is the form of the ID of a connection in TF state. If it is not globally unique, we may want to combine it with the metastore's ID to get a globally unique ID. Thoughts?

[mgyucht]

For consistency, should the Options be included here and in the next response?

[nkvuong]

done

[mgyucht]

Why is there a trailing ??

[nkvuong]

our SDK generates GET method with a trailing ? for parameters

[alexott]

Also, sometimes I see both with ? and without ? - I have few examples in the exporter

[mgyucht]

Weird... I think this is OK but probably not intentional...

[mgyucht]

Is it possible to update the user and password fields? Maybe if it were possible to see if the values for these fields in the local state is different than in the configuration, but I don't think that is possible in Terraform.

[andrewli81]

All options are updatable.

[nkvuong]

it is possible to update those options in the API, but Terraform does not allow easy updating comparing.

maybe we need a force_update option that would always send the update payload regardless of diff

[mgyucht]

Yes, but the question is how to determine whether user/password options are different locally than what is configured (imagine someone changed user/password manually in Databricks or via a separate process). How can we compute whether or not the resource needs to be updated? Typically, TF reads resources to compare them to its state & the configuration supplied by the user to compute a diff. If user and password are not returned, it may not be possible to know whether the connection needs to be updated.

[andrewli81]

Is it possible to default to always updating the username and password? There are basically 2 worlds:

1. Silently fail to update when user changes username/password
2. Always update username / password.
   I feel option 1 might be confusing to people.

[mgyucht]

Given that we have the choice between forcing users to make manual changes if secret fields are updated vs updating them every time, I agree that we should lean towards always refreshing. In light of moving forward with this resource and unblocking customers, let's do this. @nkvuong what do you think, would this be a viable option from the Terraform perspective? IIUC to do this, we treat the secret options in the same way as non-secret options, and they will be included in the diff every time.

@andrewli81 I really recommend that your team revisits the secret management story for UC connections. We don't need each team to reinvent the wheel when it comes to secret management, and when this happens, it increases complexity for consumers and results in worse user experiences. For inspiration, you can look at how secrets are referenced in Spark configurations. I understand that there is a workspace-level/account-level divide that will need to be addressed. There may be other use-cases for account-level secrets in the future as well (or even today: consider account-level service principal passwords).

[nkvuong]

we treat the secret options in the same way as non-secret options, and they will be included in the diff every time.

@alexott gave a good suggestion on how to do it, this was done for webhook
I've made the change which now works without the need for DiffSuppressFunc

[alexott]

we have similar problem with MLflow webhooks, and on read we simply restore data from the state: https://github.com/databricks/terraform-provider-databricks/blob/master/mlflow/resource_mlflow_webhook.go#L79 - we can try to do the same here

[andrewli81]

I understand that there is a workspace-level/account-level divide that will need to be addressed. There may be other use-cases for account-level secrets in the future as well (or even today: consider account-level service principal passwords).

We have to be unblocked on shipping features that need to store secrets in UC account level objects in lieu of a team that is willing to fund an account level secret manager, I will probably write a vision doc for this because I think we will need it for other use cases as well.

[mgyucht]

Completely with you. That is why I said "we should lean towards always refreshing. In light of moving forward with this resource and unblocking customers, let's do this."

[mgyucht]

What happens if a user defines two connections with the same name in two different metastores? Should the name be combined with the metastore ID to make a universally unique identifier?

[codecov-commenter]

Codecov Report

Merging #2528 (74acee9) into master (45c4d80) will decrease coverage by 0.27%.
Report is 9 commits behind head on master.
The diff coverage is 73.33%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2528      +/-   ##
==========================================
- Coverage   88.09%   87.82%   -0.27%     
==========================================
  Files         145      146       +1     
  Lines       12108    12234     +126     
==========================================
+ Hits        10667    10745      +78     
- Misses        944      968      +24     
- Partials      497      521      +24     

Files Changed	Coverage Δ
catalog/resource_catalog.go	71.76% <ø> (ø)
catalog/resource_connection.go	72.88% <72.88%> (ø)
provider/provider.go	94.11% <100.00%> (+0.03%)	⬆️

... and 9 files with indirect coverage changes

[nkvuong]

What happens if a user defines two connections with the same name in two different metastores? Should the name be combined with the metastore ID to make a universally unique identifier?

@mgyucht this is actually a good point, and we may need to retroactively do this for all UC resources 😨

[andrewli81]

CatalogInfo also has options - our open api spec needs updating.

[andrewli81]

cc @Doug-Luce

[andrewli81]

@nkvuong Can you address this one as well? Thanks!

[nkvuong]

@andrewli81 how would the options be split between connections & catalogs? and would there be sensitive options to be handled here as well?
once this is fixed in OpenAPI, we just need to regenerate the SDK, so should not take long

[andrewli81]

@nkvuong

@andrewli81 how would the options be split between connections & catalogs? and would there be sensitive options to be handled here as well?

The options of catalogs are mainly used to identify databases. This is because database systems like postgresql uses 3 layer namespaces (same as UC), so customer will need to specify which database to import into a UC catalog.

https://docs.databricks.com/en/query-federation/postgresql.html#language-SQL:~:text=Run%20the%20following%20SQL%20command%20in%20a%20notebook%20or%20Databricks%20SQL%20editor.%20Items%20in%20brackets%20are%20optional.%20Replace%20the%20placeholder%20values%3A

The open API spec has been fixed: https://docs.databricks.com/api/workspace/connections/create

Can we also fix it here?

[andrewli81]

The source of truth for the list of secret options can be retrieved by the type manifest API:
https://temp.corp.databricks.com/?114480149a090e3e#QR0tqKyrycfLQ0OH3ZEqsyZOuWEwEtNMb+o7S4bdpXs=

Can we also add personalAccessToken, access_token, client_secret, and OAuthPvtKey

[nkvuong]

done

[andrewli81]

Three comments before merging:

1. List of secret options will need to expand a bit - you can always get the list of sensitive options from the type-manifest API, I posted an example: https://temp.corp.databricks.com/?114480149a090e3e#QR0tqKyrycfLQ0OH3ZEqsyZOuWEwEtNMb+o7S4bdpXs=

I will also point future folks who add new connectors to update this list in TF.

2. Can we make the "secret" options always update if specified by user? Silently failing to update might be surprising from user perspective.

3. CreateCatalog will need options too other than connection name for foreign catalogs to work, our API spec is a bit outdated, we will update that as well.

[nkvuong]

@mgyucht all comments should be now addressed, just need to wait for the OpenAPI spec for Catalog to be updated, i.e. adding options parameter to databricks_catalog

[mgyucht]

One small question, but the implementation looks good to me.

[mgyucht]

Just to confirm: do we have any consistency around which field comes first in state? I see some places where metastore_id is first and some places where metastore_id is second. Maybe we should try to be somewhat consistent for these packed IDs.