[Phase 2] Complete the provider configuration setup #2090

fairclothjm · 2023-11-17T22:42:14Z

Description

Complete the provider configuration setup for the auth login methods.

This PR

Phase 2

Modifies the internal/provider/auth.go file to handle defaults and required fields since those can no longer be set on the Provider Schema
Modifies the auth login methods to remove defaults and make use of the new setDefaultFields method
Adds tests for the changes to default value and env var handling
Implements the Password Policy resources to show how a resource can work with the multiplexed providers
- Currently, there is an issue with an import cycle, so the tests will be implemented in a separate PR

Previous Phases

Phase 0

[Phase 0] Multiplex provider - Add multiplexer #2059
Adds a multiplexer that combines the current Terraform Plugin SDK v2 Provider implementation with 0 additional providers
Enables support for both SDKv2 and TF Plugin Framework

Phase 1

[Phase 1] Multiplex provider - minimal TF Plugin Framework provider implementation #2067
Introduces (in internal/provider/fwprovider) a minimal provider implementation that uses the TF Plugin Framework and muxes it with the current SDK v2-based provider's server. This new skeleton provider
Implements 0 resources and data sources
Is required to declare a provider schema that is identical to the schema(s) declared by all other providers in the muxed provider server
Removes Default values from the SDK v2-based provider's schema declaration as the TF Framework-based provider does not have that facility. Defaults should be handled in the provider's ConfigureFunc

Subsequent Phases

Phase 3

Fully migrate a resource/data source to the TF Plugin Framework

internal/provider/auth_token_file.go

…tion (#2097) make updates to all auth logins and fix build run go mod tidy register all auth logins to test build add all auth login blocks and match schema implementations set default for azure scope address todos - remove commented code; neither are read from env vars - add validators

austingebauer

Left some very minor suggestions and questions. Overall this is looking good!

I do wonder if there is a clean way to define the schema attributes of the various auth login methods in a single location instead of having them duplicated for both sdkv2/fw providers. That said, it might not be worth it if the change frequency is low for them. We can document that changes must match in each provider.

internal/framework/validators/gcp_test.go

internal/framework/validators/test-fixtures/fake_account.json

website/docs/index.html.markdown

internal/provider/provider.go

internal/provider/auth.go

fairclothjm · 2023-12-04T20:48:40Z

@austingebauer Thanks for the review!

clean way to define the schema attributes of the various auth login methods in a single location instead of having them duplicated for both sdkv2/fw providers

I think we could do that if we wrote a wrapper to convert to either sdkv2 or Framework but I am not convinced that it would be worth it. I do think the change frequency is pretty low. I am happy to discuss more if you like.

Also, I am working on adding tests to assert the multiplexing schemas match. terraform apply will fail if the schemas don't match but of course we want to catch it before then. :)

austingebauer · 2023-12-04T22:44:02Z

@fairclothjm - Thanks! I'm good with having the fields defined in both places provided the test can make developers aware of diffs. If it becomes a pain, we can write that conversion code at a later date 👍

benashz

Did a first pass, looks like good progress!

internal/provider/auth.go

benashz · 2023-11-20T21:27:33Z

internal/provider/auth_token_file.go

-				Required:    true,
-				DefaultFunc: schema.EnvDefaultFunc(consts.EnvVarTokenFilename, nil),
+				Type:     schema.TypeString,
+				Optional: true,


Pretty sure this is still required.

Yes, you are correct. I tested it and the behavior is the same whether we set this to Optional or Required because we assert the required fields in checkRequiredFields. I do think it makes it confusing to not set it as required here though, so I will revert these Schema changes for any required Auth Login fields.

Correction to the above which is not entirely true. For fields that we previously set from a DefaultFunc and env vars we cannot set them as Required anymore. This is because we are handling setting them from env vars after the Provider has verified all required fields are set. So, for example, if we set this field to Required we would get a Error: Missing required argument error.

I will add test cases to capture this behavior for the fields that are set via env vars.

internal/provider/auth_token_file.go

internal/provider/fwprovider/provider.go

benashz · 2023-12-05T23:30:21Z

internal/framework/client/client.go

@@ -0,0 +1,39 @@
+package client


This code looks pretty similar to what we have in the internal/provider package, would it be possible to colocate it there?

Yes, we could move it there. Initially it was doing more things that included Framework only types so I created a separate package. However, this is only intended for resources implemented with the TF Plugin Framework so I kept it in the internal/framework package for that reason. I thought it would be good to have a package that contains framework-only client helpers. I don't feel too strongly about it either way though.

benashz · 2023-12-05T23:43:10Z

internal/provider/fwprovider/provider.go

@@ -142,6 +144,18 @@ func (p *fwprovider) Schema(ctx context.Context, req provider.SchemaRequest, res
 					},
 				},
 			},
+			"auth_login":            AuthLoginGenericSchema(),


Is there a reason we can't use the auth login registry as before? That solved some issues where not all expected AuthLogin were properly set.

Good question, we might need to use that. I will look into it. Thanks!

I took a closer look at this again.

These lines define the schema for the Framework Provider (which must match 1-1 with the sdkv2 Provider). The auth login registry is still being used by the Framework Provider. The entrypoint for the auth login registry is in setClient:

terraform-provider-vault/internal/provider/meta.go

Line 255 in 852ea9a

authLogin, err := GetAuthLogin(d)

As such, there should be no behavior change for the Framework Provider since we are sharing the auth login code with the sdkv2 provider. Please let me know if you need more clarification!

internal/provider/auth.go

internal/provider/auth_aws.go

internal/provider/fwprovider/auth_aws.go

austingebauer

Left a few minor comments but generally LGTM. Nice job on this @fairclothjm!

internal/provider/fwprovider/auth_kerberos.go

internal/sys/password_policy.go

austingebauer · 2023-12-12T00:21:36Z

vault/provider_test.go

+// TestAccMuxServer uses ExternalProviders (vault) to generate a state file
+// with a previous version of the provider and then verify that there are no
+// planned changes after migrating to the Framework.


vinay-gopalan

Awesome work on this! Looks great to me, had a couple nits and questions, not blocking a merge

vinay-gopalan · 2023-12-12T01:13:07Z

internal/provider/fwprovider/auth.go

@@ -0,0 +1,61 @@
+package fwprovider


nit: sorry I forgot to add the copywrite headers for these! Definitely can let the copywrite bot take care of these before we merge to main, but just wanted to add a comment here to keep track. not a blocker

internal/provider/fwprovider/auth_kerberos.go

vinay-gopalan · 2023-12-12T01:44:23Z

internal/provider/meta_test.go

@@ -671,12 +673,32 @@ func TestNewProviderMeta(t *testing.T) {
 					},
 				},
 			},
+			env: map[string]string{


nit: I confirmed this with commenting out the env in this test and the test passes so we're definitely good here, but curious if it's worth it to add a test case for the default behavior where no env is passed

I think we are testing that in several test cases above this one

website/docs/index.html.markdown

…mplementation (#2067) * implement the framework provider * setup provider server factory * setup framework provider schema * handle defaults in configure * [Phase 2] Complete the provider configuration setup (#2090) * setup framework provider schema * handle defaults in configure * WIP: handle auth login methods * [Phase 2] Include Auth Login blocks in multiplexed Provider configuration (#2097) make updates to all auth logins and fix build run go mod tidy register all auth logins to test build add all auth login blocks and match schema implementations set default for azure scope address todos - remove commented code; neither are read from env vars - add validators * remove set_namespace_from_token field * add custom path validator * configure namespace for vault client * review comments * add muxed provider acc tests * use consts, set required fields and update tests * add tests for multi envs and fix bug * fix bug with overriding config val * add custom URI validator for OIDC auth * fix a few default mount types * add comments and migration state example acc test * remove dupe default entries; add validators * remove client_auth docs * add kerbos token and file path validators * don't use double pointer and remove redundant return --------- Co-authored-by: vinay-gopalan <86625824+vinay-gopalan@users.noreply.github.com> --------- Co-authored-by: vinay-gopalan <86625824+vinay-gopalan@users.noreply.github.com>

fairclothjm added 3 commits November 15, 2023 10:52

setup framework provider schema

7a5b20d

handle defaults in configure

8e37102

WIP: handle auth login methods

78f1c0c

github-actions bot added the size/XL label Nov 17, 2023

vinay-gopalan reviewed Nov 17, 2023

View reviewed changes

internal/provider/auth_token_file.go Outdated Show resolved Hide resolved

vinay-gopalan mentioned this pull request Nov 22, 2023

[Phase 2] Include Auth Login blocks in multiplexed Provider configuration #2097

Merged

github-actions bot added dependencies documentation labels Nov 29, 2023

remove set_namespace_from_token field

fe6136b

fairclothjm force-pushed the VAULT-21288/auth-login-fwprovider branch from 3799f9a to fe6136b Compare November 30, 2023 18:05

add custom path validator

9887771

austingebauer reviewed Dec 4, 2023

View reviewed changes

configure namespace for vault client

9f607e2

fairclothjm force-pushed the VAULT-21288/auth-login-fwprovider branch from 70ea03b to 9f607e2 Compare December 4, 2023 20:26

review comments

fc3d003

add muxed provider acc tests

852ea9a

benashz reviewed Dec 5, 2023

View reviewed changes

fairclothjm added 6 commits December 6, 2023 16:18

use consts, set required fields and update tests

0c57be6

add tests for multi envs and fix bug

0a2b62d

fix bug with overriding config val

b7cff13

add custom URI validator for OIDC auth

7a05466

fix a few default mount types

970bebc

add comments and migration state example acc test

9e098a2

fairclothjm marked this pull request as ready for review December 8, 2023 16:50

fairclothjm changed the title ~~WIP - [Phase 2] Complete the provider configuration setup~~ [Phase 2] Complete the provider configuration setup Dec 8, 2023

austingebauer reviewed Dec 8, 2023

View reviewed changes

internal/provider/auth_aws.go Outdated Show resolved Hide resolved

internal/provider/auth_aws.go Outdated Show resolved Hide resolved

internal/provider/fwprovider/auth_aws.go Outdated Show resolved Hide resolved

internal/provider/fwprovider/auth_aws.go Outdated Show resolved Hide resolved

remove dupe default entries; add validators

46471c2

austingebauer approved these changes Dec 12, 2023

View reviewed changes

vinay-gopalan approved these changes Dec 12, 2023

View reviewed changes

fairclothjm added 3 commits December 12, 2023 13:21

remove client_auth docs

de76827

add kerbos token and file path validators

3fbf25a

don't use double pointer and remove redundant return

76e5279

fairclothjm merged commit bbf6580 into VAULT-21287/minimum-fwprovider Dec 12, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Phase 2] Complete the provider configuration setup #2090

[Phase 2] Complete the provider configuration setup #2090

fairclothjm commented Nov 17, 2023 •

edited

Loading

austingebauer left a comment

fairclothjm commented Dec 4, 2023

austingebauer commented Dec 4, 2023

benashz left a comment

benashz Nov 20, 2023

fairclothjm Dec 6, 2023

fairclothjm Dec 6, 2023 •

edited

Loading

fairclothjm Dec 6, 2023

benashz Dec 5, 2023

fairclothjm Dec 6, 2023

benashz Dec 5, 2023

fairclothjm Dec 6, 2023

fairclothjm Dec 6, 2023

austingebauer left a comment

austingebauer Dec 12, 2023

vinay-gopalan left a comment

vinay-gopalan Dec 12, 2023

vinay-gopalan Dec 12, 2023

fairclothjm Dec 12, 2023

[Phase 2] Complete the provider configuration setup #2090

[Phase 2] Complete the provider configuration setup #2090

Conversation

fairclothjm commented Nov 17, 2023 • edited Loading

Description

This PR

Phase 2

Previous Phases

Phase 0

Phase 1

Subsequent Phases

Phase 3

austingebauer left a comment

Choose a reason for hiding this comment

fairclothjm commented Dec 4, 2023

austingebauer commented Dec 4, 2023

benashz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fairclothjm Dec 6, 2023 • edited Loading

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

austingebauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinay-gopalan left a 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

fairclothjm commented Nov 17, 2023 •

edited

Loading

fairclothjm Dec 6, 2023 •

edited

Loading