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

1. Add support for gcp cmk #2015

Merged
merged 3 commits into from
Feb 22, 2023
Merged

1. Add support for gcp cmk #2015

merged 3 commits into from
Feb 22, 2023

Conversation

rohankabra-db
Copy link
Contributor

Tested acceptance test by running bash-3.2$ scripts/run.sh gcp-accounts 'TestGcpaAccCustomerManagedKeysForStorage' --debug --tee -



3 tf_rpc=ApplyResourceChange
2023-02-16T17:10:32.183-0800 [TRACE] sdk.helper_schema: Calling downstream: tf_resource_type=databricks_mws_customer_managed_keys tf_req_id=d4d9dd50-0291-96e6-a6b4-32e2ae934e85 tf_provider_addr=registry.terraform.io/hashicorp/databricks tf_rpc=ApplyResourceChange
2023/02/16 17:10:32 [INFO] Configured google-accounts auth: host=https://accounts.staging.gcp.databricks.com, account_id=9fcbb245-7c44-4522-9870-e38324104cf8, google_service_account=rohansa2-staging@databricks-staging-customer.iam.gserviceaccount.com
2023/02/16 17:10:33 [DEBUG] POST /api/2.0/accounts/9fcbb245-7c44-4522-9870-e38324104cf8/customer-managed-keys {
  "account_id": "9fcbb245-7c44-4522-9870-e38324104cf8",
  "gcp_key_info": {
    "kms_key_id": "projects/databricks-staging-customer/locations/us-central1/keyRings/cmk-qa/cryptoKeys/cmk-qa-key"
  },
  "use_cases": [
    "STORAGE"
  ]
}
2023/02/16 17:10:34 [DEBUG] 201 Created  {
  "account_id": "9fcbb245-7c44-4522-9870-e38324104cf8",
  "creation_time": 1676596234145,
  "customer_managed_key_id": "77b97eae-24eb-422e-90ed-f36f1540acc5",
  "gcp_key_info": {
    "kms_key_id": "projects/databricks-staging-customer/locations/us-central1/keyRings/cmk-qa/cryptoKeys/cmk-qa-key"
  },
  "updated_time": 1676596234145,
  "use_cases": [
    "STORAGE"
  ]
} <- POST /api/2.0/accounts/9fcbb245-7c44-4522-9870-e38324104cf8/customer-managed-keys
2023/02/16 17:10:34 [DEBUG] GET /api/2.0/accounts/9fcbb245-7c44-4522-9870-e38324104cf8/customer-managed-keys/77b97eae-24eb-422e-90ed-f36f1540acc5
2023/02/16 17:10:34 [DEBUG] 200 OK  {
  "account_id": "9fcbb245-7c44-4522-9870-e38324104cf8",
  "creation_time": 1676596234145,
  "customer_managed_key_id": "77b97eae-24eb-422e-90ed-f36f1540acc5",
  "gcp_key_info": {
    "kms_key_id": "projects/databricks-staging-customer/locations/us-central1/keyRings/cmk-qa/cryptoKeys/cmk-qa-key"
  },
  "updated_time": 1676596234145,
  "use_cases": [
    "STORAGE"
  ]
} <- GET /api/2.0/accounts/9fcbb245-7c44-4522-9870-e38324104cf8/customer-managed-keys/77b97eae-24eb-422e-90ed-f36f1540acc5
2023-02-16T17:10:34.609-0800 [TRACE] sdk.helper_schema: Called downstream: tf_provider_addr=registry.terraform.io/hashicorp/databricks tf_rpc=ApplyResourceChange tf_resource_type=databricks_mws_customer_managed_keys tf_req_id=d4d9dd50-0291-96e6-a6b4-32e2ae934e85

@alexott alexott requested review from a team February 17, 2023 08:13
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.

Also, documentation for this resource wasn't updated to describe it. Also, please say that either AWS or GCP is required

// CustomerManagedKey contains key information and metadata for BYOK for E2
type CustomerManagedKey struct {
CustomerManagedKeyID string `json:"customer_managed_key_id,omitempty" tf:"computed"`
AwsKeyInfo *AwsKeyInfo `json:"aws_key_info" tf:"force_new"`
AwsKeyInfo *AwsKeyInfo `json:"aws_key_info,omitempty" tf:"force_new"`
Copy link
Contributor

Choose a reason for hiding this comment

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

it makes sense to add conflicts: annotations to each of the fields, so people won't be able to specify both AWS & GCP keys

Copy link
Contributor

Choose a reason for hiding this comment

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

backend validates it anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexott
Made the changes as suggested. Added conflicts to both AwsKeyInfo and GcpKeyInfo. Please verify. Will send the new revision.

// CustomerManagedKey contains key information and metadata for BYOK for E2
// You must specify either AwsKeyInfo for AWS or GcpKeyInfo for GCP, but not both
type CustomerManagedKey struct {
	CustomerManagedKeyID string      `json:"customer_managed_key_id,omitempty" tf:"computed"`
	AwsKeyInfo           *AwsKeyInfo `json:"aws_key_info,omitempty" tf:"force_new,conflicts:gcp_key_info"`
	GcpKeyInfo           *GcpKeyInfo `json:"gcp_key_info,omitempty" tf:"force_new,conflicts:aws_key_info"`
	AccountID            string      `json:"account_id" tf:"force_new"`
	CreationTime         int64       `json:"creation_time,omitempty" tf:"computed"`
	UseCases             []string    `json:"use_cases"`
}

@rohankabra-db
Copy link
Contributor Author

@alexott @nfx The documentation here needs to be changed completely to make it cloud agnostic. At present, it just references AWS docs and AWS way of doing it.

We don't have any public facing GCP docs to share yet as the feature is still in private preview. Should we do this change later?

@alexott
Copy link
Contributor

alexott commented Feb 21, 2023

yes, it's ok to leave out documentation until the public preview

2. Add details to gcp guide for authentication with service account (SA-1)
@rohankabra-db
Copy link
Contributor Author

@alexott Addressed your comments. Please take a look whenever you get time.

@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2023

Codecov Report

Merging #2015 (232fe30) into master (f7967ca) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2015   +/-   ##
=======================================
  Coverage   90.34%   90.34%           
=======================================
  Files         136      136           
  Lines       10887    10887           
=======================================
  Hits         9836     9836           
  Misses        665      665           
  Partials      386      386           
Impacted Files Coverage Δ
mws/resource_mws_customer_managed_keys.go 92.10% <ø> (ø)

@nfx nfx merged commit dd3766c into databricks:master Feb 22, 2023
@nfx nfx mentioned this pull request Feb 24, 2023
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