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

Fix create storage credentials with owner for account #3184

Merged

Conversation

donatasm
Copy link
Contributor

Changes

For the given account storage credentials definition:

resource "databricks_storage_credential" "catalog_staging" {
  name  = "catalog_staging"
  owner = databricks_service_principal.catalog["staging"].application_id
  metastore_id = databricks_metastore.unity_metastore.id
  aws_iam_role {
    role_arn = "arn:aws:iam::1234567890:role/MyRole-AJJHDSKSDF"
  }
  provider = databricks.account
}

fixes the error:

Error: cannot create storage credential: Storage Credential '02e010b4-0fdb-46d7-af9f-706e99f2b19b' does not exist.

as described in #3134 (comment).

From the logs, before this change, storage credential ID is used instead of a name when calling PUT:

2024-01-19T13:34:51.868Z [DEBUG] provider.terraform-provider-databricks_v1.34.0: POST /api/2.0/accounts/b0834ee0-0ead-4165-9d0a-6f081fdae9bc/metastores/79ba06d5-3c04-4fc1-9df2-a82ac2ec27e4/storage-credentials
> {
>   "credential_info": {
>     "aws_iam_role": {
>       "role_arn": "arn:aws:iam::1234567890:role/MyRole-AJJHDSKSDF"
>     },
>     "name": "catalog_staging"
>   }
> }
< HTTP/2.0 200 OK
< {
<   "credential_info": {
<     "aws_iam_role": {
<       "external_id": "b0834ee0-0ead-4165-9d0a-6f081fdae9bc",
<       "role_arn": "arn:aws:iam::1234567890:role/MyRole-AJJHDSKSDF",
<       "unity_catalog_iam_arn": "arn:aws:iam::414351767826:role/unity-catalog-prod-UCMasterRole-14S5ZJVKOTYTL"
<     },
<     "created_at": 1705671291796,
<     "created_by": "[email protected]",
<     "full_name": "catalog_staging",
<     "id": "02e010b4-0fdb-46d7-af9f-706e99f2b19b",
<     "isolation_mode": "ISOLATION_MODE_OPEN",
<     "metastore_id": "79ba06d5-3c04-4fc1-9df2-a82ac2ec27e4",
<     "name": "catalog_staging",
<     "owner": "[email protected]",
<     "read_only": false,
<     "securable_kind": "STORAGE_CREDENTIAL_AWS_IAM",
<     "securable_type": "STORAGE_CREDENTIAL",
<     "updated_at": 1705671291796,
<     "updated_by": "[email protected]"
<   }
< }: tf_provider_addr=registry.terraform.io/databricks/databricks tf_rpc=ApplyResourceChange @caller=/home/runner/work/terraform-provider-databricks/terraform-provider-databricks/logger/logger.go:33 @module=databricks tf_req_id=3be0adc2-8aa6-6a10-3bf7-5f2b15ecde33 tf_resource_type=databricks_storage_credential timestamp=2024-01-19T13:34:51.868Z
2024-01-19T13:34:52.457Z [DEBUG] provider.terraform-provider-databricks_v1.34.0: non-retriable error: Storage Credential '02e010b4-0fdb-46d7-af9f-706e99f2b19b' does not exist.: tf_provider_addr=registry.terraform.io/databricks/databricks tf_resource_type=databricks_storage_credential tf_rpc=ApplyResourceChange @caller=/home/runner/work/terraform-provider-databricks/terraform-provider-databricks/logger/logger.go:33 @module=databricks tf_req_id=3be0adc2-8aa6-6a10-3bf7-5f2b15ecde33 timestamp=2024-01-19T13:34:52.457Z
2024-01-19T13:34:52.457Z [DEBUG] provider.terraform-provider-databricks_v1.34.0: PUT /api/2.0/accounts/b0834ee0-0ead-4165-9d0a-6f081fdae9bc/metastores/79ba06d5-3c04-4fc1-9df2-a82ac2ec27e4/storage-credentials/02e010b4-0fdb-46d7-af9f-706e99f2b19b
> {
>   "credential_info": {
>     "aws_iam_role": {
>       "role_arn": "arn:aws:iam::1234567890:role/MyRole-AJJHDSKSDF"
>     },
>     "owner": "0c6b1aa7-c609-4605-9195-710ec90cb478"
>   }
> }
< HTTP/2.0 404 Not Found
< {
<   "details": [
<     {
<       "@type": "type.googleapis.com/google.rpc.RequestInfo",
<       "request_id": "f31c49ab-ab2f-4d13-9275-16df29b08747",
<       "serving_data": ""
<     }
<   ],
<   "error_code": "STORAGE_CREDENTIAL_DOES_NOT_EXIST",
<   "message": "Storage Credential '02e010b4-0fdb-46d7-af9f-706e99f2b19b' does not exist."

which leads to STORAGE_CREDENTIAL_DOES_NOT_EXIST error, because storage_credential_name is expected, not the ID.

This change passes storage credential name instead of an ID.

Tests

Tested on live Databricks account with the patched version of the provider. Account REST API behavior captured in basic unit test.

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

lgtm

@alexott alexott requested a review from nkvuong February 2, 2024 09:37
Copy link
Contributor

@nkvuong nkvuong left a comment

Choose a reason for hiding this comment

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

do you mind adding an acceptance test with owner under internal/acceptance/storage_credential_test.go

@nkvuong nkvuong force-pushed the fix-create-storage-credentials-with-owner branch from a763dc8 to 3ae081b Compare February 2, 2024 15:55
@donatasm
Copy link
Contributor Author

donatasm commented Feb 5, 2024

do you mind adding an acceptance test with owner under internal/acceptance/storage_credential_test.go

@nkvuong I've added initial version of the acceptance test, please take a look.

@donatasm donatasm requested a review from nkvuong February 5, 2024 07:57
@nkvuong
Copy link
Contributor

nkvuong commented Feb 6, 2024

integration tests passed

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4213468) 83.57% compared to head (3376ba9) 83.71%.
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3184      +/-   ##
==========================================
+ Coverage   83.57%   83.71%   +0.13%     
==========================================
  Files         168      169       +1     
  Lines       15083    15094      +11     
==========================================
+ Hits        12606    12636      +30     
+ Misses       1736     1715      -21     
- Partials      741      743       +2     
Files Coverage Δ
catalog/resource_storage_credential.go 59.74% <100.00%> (+12.33%) ⬆️

... and 2 files with indirect coverage changes

@alexott alexott added this pull request to the merge queue Feb 7, 2024
Merged via the queue into databricks:main with commit 2606c76 Feb 7, 2024
5 checks passed
@donatasm donatasm deleted the fix-create-storage-credentials-with-owner branch February 7, 2024 09:23
@edwardfeng-db edwardfeng-db mentioned this pull request Feb 7, 2024
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.

4 participants