Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Add databricks_credential resource #4219

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

nkvuong
Copy link
Contributor

@nkvuong nkvuong commented Nov 13, 2024

Changes

Resolves #4214

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


A credential represents an authentication and authorization mechanism for accessing services on your cloud tenant. Each credential is subject to Unity Catalog access-control policies that control which users and groups can access the credential.

To create credentials, you must be a Databricks account admin or have the `CREATE SERVICE CREDENTIAL` privilege. The user who creates the credential can delegate ownership to another user or group to manage permissions on it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will create service credential work with service principals on azure?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also mention CREATE_STORAGE_CREDENTIAL ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the SDK only accepts SERVICE for purpose apparently

Copy link
Contributor Author

@nkvuong nkvuong Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our go-sdk struct does not match the OpenAPI spec for some reason...maybe we need to wait for it to be regenerated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our go-sdk struct does not match the OpenAPI spec for some reason...maybe we need to wait for it to be regenerated

I think that's the issue here. We recently updated the proto files (and hence the OpenAPI specs) to include the management of storage credentials with the /credentials endpoints

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will create service credential work with service principals on azure?

No. We only support Azure Managed Identities

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also mention CREATE_STORAGE_CREDENTIAL ?

yes, we should. See the docs https://docs.databricks.com/api/workspace/credentials/createcredential

docs/resources/grant.md Show resolved Hide resolved
docs/resources/credential.md Show resolved Hide resolved
@alexott
Copy link
Contributor

alexott commented Nov 13, 2024

catalog/resource_credential.go Show resolved Hide resolved

A credential represents an authentication and authorization mechanism for accessing services on your cloud tenant. Each credential is subject to Unity Catalog access-control policies that control which users and groups can access the credential.

To create credentials, you must be a Databricks account admin or have the `CREATE SERVICE CREDENTIAL` privilege. The user who creates the credential can delegate ownership to another user or group to manage permissions on it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs/resources/credential.md Show resolved Hide resolved
docs/resources/credential.md Show resolved Hide resolved
docs/resources/grant.md Show resolved Hide resolved
- `name` - Name of Credentials, which must be unique within the [databricks_metastore](metastore.md). Change forces creation of a new resource.
- `purpose` - Indicates the purpose of the credential. Can be `SERVICE`.
- `owner` - (Optional) Username/groupname/sp application_id of the credential owner.
- `read_only` - (Optional) Indicates whether the credential is only usable for read operations.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this only applies to storage credentials


- `name` - Name of Credentials, which must be unique within the [databricks_metastore](metastore.md). Change forces creation of a new resource.
- `purpose` - Indicates the purpose of the credential. Can be `SERVICE`.
- `owner` - (Optional) Username/groupname/sp application_id of the credential owner.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik, the owner can't be set. It's derived from the identity of the user creating the credential

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless this also applies to updates

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this applies to update - and in case of create, we execute an update as well

The following arguments are required:

- `name` - Name of Credentials, which must be unique within the [databricks_metastore](metastore.md). Change forces creation of a new resource.
- `purpose` - Indicates the purpose of the credential. Can be `SERVICE`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `purpose` - Indicates the purpose of the credential. Can be `SERVICE`.
- `purpose` - Indicates the purpose of the credential. Can be `SERVICE` or `STORAGE`.

- `skip_validation` - (Optional) Suppress validation errors if any & force save the credential.
- `force_destroy` - (Optional) Delete credential regardless of its dependencies.
- `force_update` - (Optional) Update credential regardless of its dependents.
- `isolation_mode` - (Optional) Whether the credential is accessible from all workspaces or a specific set of workspaces. Can be `ISOLATION_MODE_ISOLATED` or `ISOLATION_MODE_OPEN`. Setting the credential to `ISOLATION_MODE_ISOLATED` will automatically allow access from the current workspace.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `isolation_mode` - (Optional) Whether the credential is accessible from all workspaces or a specific set of workspaces. Can be `ISOLATION_MODE_ISOLATED` or `ISOLATION_MODE_OPEN`. Setting the credential to `ISOLATION_MODE_ISOLATED` will automatically allow access from the current workspace.
- `isolation_mode` - (Optional) Whether the credential is accessible from all workspaces or a specific set of workspaces. Can be `ISOLATION_MODE_ISOLATED` or `ISOLATION_MODE_OPEN`. Setting the credential to `ISOLATION_MODE_ISOLATED` will automatically restrict access to only from the current workspace.

When isolation_mode is changed, do we implicitly make a call to bind the credential to the current workspace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, otherwise TF cannot manage the credential - I don't want to say "automatically restrict access to only from the current workspace", as customers will add additional bindings as necessary via the databricks_workspace_binding resource

@@ -0,0 +1,96 @@
---

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those are data sources, we need to implement them in a separate PR

Copy link

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/terraform

Inputs:

  • PR number: 4219
  • Commit SHA: 615b20e1bcdacc9fde45f7e27db0b4e4554d032f

Checks will be approved automatically on success.

@eng-dev-ecosystem-bot
Copy link
Collaborator

Test Details: go/deco-tests/11869025907

Copy link

@farrucosanjurjo-db farrucosanjurjo-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I've no experience building terraform providers. My assessment is about the payloads, fields, usage comments, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Add support for UC Credentials
5 participants