replace TokenFile with direct Token in provider configuration #1053

liamcervante · 2024-07-22T11:38:03Z

🛠️ Description

For Stacks, we want to support the ability to process the JWT directly instead of loading it via a file. This PR updates the provider configuration so that users can choose to specify the token directly instead of writing it into a file first. The old approach of writing into a file is still available.

dadgar · 2024-08-05T22:44:38Z

internal/provider/provider.go

+							Optional:    true,
+							Description: "The path to a file containing a JWT token retrieved from an OpenID Connect (OIDC) or OAuth2 provider. Exactly one of `token_file` or `token` must be set.",
+							Validators: []validator.String{
+								stringvalidator.LengthAtLeast(1),


Could we use stringvalidator.ConflictsWith or something similar to have TF validate only one is set?

Nice, didn't know they had such specific validators. I did end up using ExactlyOneOf instead of just ConflictsWith - I think that has the same effect but with an additional validation that one must be set.

liamcervante · 2024-08-07T08:28:02Z

I've tested this locally, and all is working 🎉

pierluc-codes · 2024-08-08T15:04:39Z

docs/index.md

+
+Optional:
+
+- `token` (String) The JWT token retrieved from an OpenID Connect (OIDC) or OAuth2 provider. Exactly one of `token_file` or `token` must be set.


What is the motivation of doing an exactly one?

This is two different ways of specifying the same information. Either the token has been written to disk, in which case you should specify the token_file variable, or the token has been loaded dynamically and Terraform just has it, in which case you should specify it directly via token.

It doesn't make sense to specify both of these together. Similarly, you must specify the token somehow if you are authenticating via the block, so you must specify at least one.

Letting the user specifying both with a priority would simplify the conditional. I understand the need for at least once. As an example, firebase/firebase-tool: https://github.com/firebase/firebase-tools?tab=readme-ov-file#general

Ultimately, I don't mind having a exactly once, I would like to know the why behind it.

I've changed this to AtLeastOneOf, and the token field takes precedence over the token_file field.

pierluc-codes · 2024-08-08T15:05:05Z

internal/clients/client.go

@@ -122,18 +127,35 @@ func NewClient(config ClientConfig) (*Client, error) {
 		opts = append(opts, hcpConfig.WithClientCredentials(config.ClientID, config.ClientSecret))
 	} else if config.CredentialFile != "" {
 		opts = append(opts, hcpConfig.WithCredentialFilePath(config.CredentialFile))
-	} else if config.WorloadIdentityTokenFile != "" && config.WorkloadIdentityResourceName != "" {
+	} else if (config.WorkloadIdentityToken != "" || config.WorkloadIdentityTokenFile != "") && config.WorkloadIdentityResourceName != "" {


This is hard to parse for my out-of-context brain. Is it possible to do it better?

I've pulled out the combined or logic into a standalone variable, and added some additional comments explaining the flow. Hopefully, that is better?

pierluc-codes · 2024-08-08T15:05:41Z

internal/clients/vault_secrets_preview.go

@@ -101,15 +101,15 @@ func GetRotatingSecretState(ctx context.Context, client *Client, loc *sharedmode

 // CreateMongoDBAtlasRotationIntegration NOTE: currently just needed for tests
 func CreateMongoDBAtlasRotationIntegration(ctx context.Context, client *Client, loc *sharedmodels.HashicorpCloudLocationLocation, integrationName, mongodbAtlasPublicKey, mongodbAtlasPrivateKey string) (*secretmodels.Secrets20231128MongoDBAtlasIntegration, error) {
-	body := secret_service.CreateMongoDBAtlasIntegrationBody{
+	body := secretmodels.SecretServiceCreateMongoDBAtlasIntegrationBody{


Can you give some context for those changes?

These are changes forced by the hcp-sdk-go upgrade that also added in the new token input when authenticating. The old approach (via the secret_service package) has gone and so wasn't compiling after the upgrade.

There's no functional changes here, the location and names of the various structs have simply been moved around in the referenced library, so the code needs to be updated to point to the new locations.

pierluc-codes

I don't see any added or updated test

pierluc-codes · 2024-08-08T16:12:12Z

docs/index.md

+
+Optional:
+
+- `token` (String) The JWT token retrieved from an OpenID Connect (OIDC) or OAuth2 provider. Exactly one of `token_file` or `token` must be set.


Letting the user specifying both with a priority would simplify the conditional. I understand the need for at least once. As an example, firebase/firebase-tool: https://github.com/firebase/firebase-tools?tab=readme-ov-file#general

Ultimately, I don't mind having a exactly once, I would like to know the why behind it.

pierluc-codes · 2024-08-08T16:13:09Z

internal/clients/client.go

@@ -116,24 +121,47 @@ type ClientConfig struct {

 // NewClient creates a new Client that is capable of making HCP requests
 func NewClient(config ClientConfig) (*Client, error) {
+	hasWorkloadIdentityToken := config.WorkloadIdentityToken != "" || config.WorkloadIdentityTokenFile != ""


I like that approach of add boolean, i think it would be beneficial to add even more which will make your intention more clear and the make the code a little bit more self documenting.

I've added more in 👍

liamcervante · 2024-08-09T09:34:01Z

I don't see any added or updated test

This is very difficult to test with an automated process. It's why there weren't any automatic tests for the previous implementation which just supported the token_file input.

We'd need to:

Generate a public/private key pair.
Host and operate an OAuth / OIDC exchange service that publishes the public key.
Configure a HCP account that trusts our new OAuth / OIDC service
Ask for the OAuth / OIDC service to generate a JWT using the private key
Then we can test the provider by inputting that JWT into the token or token_file fields

I've tested this manually by modifying TFC / Atlas / tfc-agent so that I can get a JWT from there which usually isn't possible, and definitely not something we should allow in normal operation. I then built the provider locally, and tested Terraform operations using the JWT I hacked out of TFC with my HCP account set up to trust TFC.

I could add some unit tests for this, but the client actually initiates the token exchange as part of it's initialisation, so we'd need to create some kind of mock or move the token exchange out of initialisation and refactor everything to call the token exchange after.

pierluc-codes · 2024-08-09T16:04:05Z

This is very difficult to test with an automated process.

I would be minimally happy with only unit test that validate that the expected output are created from the given input.

liamcervante · 2024-08-12T10:31:44Z

Added unit tests!

liamcervante added 2 commits July 22, 2024 13:32

replace TokenFile with direct Token in provider configuration

ffbb2f7

replace TokenFile with direct Token in provider configuration

bc7a1a0

liamcervante requested a review from dadgar July 22, 2024 11:38

liamcervante added 5 commits August 5, 2024 11:11

Merge branch 'hashicorp:main' into main

13a1eea

Merge branch 'main' of github.com:liamcervante/terraform-provider-hcp

413955f

ready for review

dc51271

fix for latest sdk and go generate

a4168f4

move changelog to latest

ac1359f

liamcervante force-pushed the main branch from 99d6eb3 to ac1359f Compare August 5, 2024 09:31

liamcervante marked this pull request as ready for review August 5, 2024 09:35

liamcervante requested review from a team as code owners August 5, 2024 09:35

dadgar reviewed Aug 5, 2024

View reviewed changes

use ExactlyOneOf to validate token and token_file

92f2014

liamcervante requested a review from dadgar August 6, 2024 09:43

dadgar approved these changes Aug 6, 2024

View reviewed changes

dhuckins approved these changes Aug 8, 2024

View reviewed changes

pierluc-codes reviewed Aug 8, 2024

View reviewed changes

address comments

9d37b08

liamcervante requested a review from pierluc-codes August 8, 2024 15:38

pierluc-codes reviewed Aug 8, 2024

View reviewed changes

liamcervante added 3 commits August 9, 2024 10:23

add more variables

06840f5

change exactly one of to at least one of

5b1596b

update docs

97f64d9

liamcervante requested a review from pierluc-codes August 9, 2024 09:34

add unit tests

8883d81

liamcervante force-pushed the main branch from c0e6a49 to 8883d81 Compare August 12, 2024 10:31

Merge branch 'main' into main

0fce2df

pierluc-codes approved these changes Aug 14, 2024

View reviewed changes

liamcervante merged commit ac907e1 into hashicorp:main Aug 15, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace TokenFile with direct Token in provider configuration #1053

replace TokenFile with direct Token in provider configuration #1053

liamcervante commented Jul 22, 2024 •

edited

Loading

dadgar Aug 5, 2024

liamcervante Aug 6, 2024

liamcervante commented Aug 7, 2024

pierluc-codes Aug 8, 2024

liamcervante Aug 8, 2024

pierluc-codes Aug 8, 2024

liamcervante Aug 9, 2024

pierluc-codes Aug 8, 2024

liamcervante Aug 8, 2024

pierluc-codes Aug 8, 2024

liamcervante Aug 8, 2024

pierluc-codes left a comment

pierluc-codes Aug 8, 2024

pierluc-codes Aug 8, 2024 •

edited

Loading

liamcervante Aug 9, 2024

liamcervante commented Aug 9, 2024

pierluc-codes commented Aug 9, 2024

liamcervante commented Aug 12, 2024


		Optional:

		- `token` (String) The JWT token retrieved from an OpenID Connect (OIDC) or OAuth2 provider. Exactly one of `token_file` or `token` must be set.

replace TokenFile with direct Token in provider configuration #1053

replace TokenFile with direct Token in provider configuration #1053

Conversation

liamcervante commented Jul 22, 2024 • edited Loading

🛠️ Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamcervante commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierluc-codes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierluc-codes Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamcervante commented Aug 9, 2024

pierluc-codes commented Aug 9, 2024

liamcervante commented Aug 12, 2024

liamcervante commented Jul 22, 2024 •

edited

Loading

pierluc-codes Aug 8, 2024 •

edited

Loading