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.

Sign up for GitHub

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

Jump to bottom

Fix Azure SP Personal Access Token Duration #110

Merged
merged 8 commits into from Jun 25, 2020
Merged

Fix Azure SP Personal Access Token Duration #110

merged 8 commits into from Jun 25, 2020

Conversation

stikkireddy
Copy link
Contributor

@stikkireddy stikkireddy commented Jun 16, 2020 •
edited

This lets the user control the duration of PAT token that the user creates. it introduces a new field
pat_token_duration_seconds to the azure_auth block in the provider.

  • Changed the token create client to accept time.Duration rather than int32
  • Made the pat token duration a field in the azure_auth block but will be deprecated for preference of the AAD token when it fully supports all the Databricks APIs
  • Added DBApiClientMetadata to the DBApiClientConfig to help with testing
  • Added docs for the field pat_token_duration_seconds

Fixes #66

@stikkireddy
switched the client api to create token to accept time.duration rathe…
a3188cc 
…r than int32. azure_auth now supports pat_token_duration_seconds to customize the temporary pat token. It will get deprecated when full support of AAD is released.
@stikkireddy
Copy link
Contributor Author

@sdebruyn this should resolve your issue and let the duration be customizable please take a quick look at this and if you have any comments on the new field name please let me know.

@stikkireddy stikkireddy requested a review from nfx June 16, 2020 22:46
@stikkireddy stikkireddy mentioned this pull request Jun 16, 2020
Closed
@sdebruyn
Copy link
Contributor

LGTM!

@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2020 •
edited

Codecov Report

Merging #110 into master will decrease coverage by 0.00%.
The diff coverage is 43.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
- Coverage   48.85%   48.85%   -0.01%     
==========================================
  Files          60       60              
  Lines        7420     7437      +17     
==========================================
+ Hits         3625     3633       +8     
- Misses       3719     3728       +9     
  Partials       76       76
Flag Coverage Δ
#client 73.56% <100.00%> (ø)
#provider 41.84% <38.09%> (+0.01%) ⬆️
#repository 48.85% <43.47%> (-0.01%) ⬇️
Impacted Files Coverage Δ
client/service/client.go 66.33% <ø> (ø)
databricks/azure_auth.go 0.00% <0.00%> (ø)
databricks/resource_databricks_token.go 50.74% <0.00%> (-0.77%) ⬇️
databricks/provider.go 71.96% <57.14%> (-0.84%) ⬇️
client/service/tokens.go 100.00% <100.00%> (ø)

nfx
nfx requested changes Jun 19, 2020
View reviewed changes
Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

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

I do not agree we need changes like this in provider. there's databricks_token method that generates PAT and let it be the only thing that generates PATs for both MWS & Azure workspaces. It alone has stability issues in this provider.

client/service/client.go Outdated Show resolved Hide resolved
databricks/azure_auth.go Outdated
@@ -36,6 +36,7 @@ type TokenPayload struct {
ClientSecret string
ClientID string
TenantID string
PatTokenDuration int32
Copy link
Contributor

Choose a reason for hiding this comment

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

this does not logically belong to azure auth, it's a mix of concerns here

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 will be removed when support of secret scopes are available for AAD, it can be removed from the struct here and the workspace token function can be removed entirely. It should be transparent to the user if we choose to deprecate the pat_token_duration_seconds field and not use it in the code.

Needed some way to capture that from the schema. Currently the goal of the azure_auth functionality is to generate a pat token for users to use that service principal to provision resources. So we can keep this till that dependency is met.

This is not ideal but we can add an enhancement in the future to refresh this token so duration is not required field and that can be also transparent to the user.

@stikkireddy
Merge branches 'azure-sp-token-duration-fix' and 'master' of github.c…
f90a90b 
…om:databrickslabs/terraform-provider-databricks into azure-sp-token-duration-fix

� Conflicts:
�	client/service/tokens.go
�	databricks/azure_auth.go
�	databricks/provider.go
�	databricks/resource_databricks_token.go
@stikkireddy
removed the DBApiClientMetadata and added TokenCreateTime field for e…
a32def4 
…nabling testing, added default of 3600 seconds for pat token and added test for the default func; added docs
@TravisBuddy
Copy link

Hey @stikkireddy,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 9cd1e320-b6b8-11ea-aaf6-456b40a357ff

@TravisBuddy
Copy link

Hey @stikkireddy,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 0d3ff8d0-b6ba-11ea-aaf6-456b40a357ff

@TravisBuddy
Copy link

Hey @stikkireddy,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 6506faf0-b6ba-11ea-aaf6-456b40a357ff

@stikkireddy
Merge branches 'azure-sp-token-duration-fix' and 'master' of github.c…
9715850 
…om:databrickslabs/terraform-provider-databricks into azure-sp-token-duration-fix

� Conflicts:
�	databricks/azure_auth.go
@TravisBuddy
Copy link

Hey @stikkireddy,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 51473000-b6e4-11ea-bb0d-6965ce86ea0c

nfx
nfx approved these changes Jun 25, 2020
View reviewed changes
Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@nfx nfx merged commit 15b5505 into master Jun 25, 2020
@nfx nfx deleted the azure-sp-token-duration-fix branch June 25, 2020 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfx nfx nfx approved these changes

Assignees

@stikkireddy stikkireddy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ISSUE] Databricks provider uses invalid access token
5 participants
@stikkireddy @sdebruyn @codecov-commenter @TravisBuddy @nfx