feat: Secret resource #3110

sfc-gh-fbudzynski · 2024-10-02T13:56:50Z

Changes

add snowflake_secret_with_client_credentials resource
add snowflake_secret_with_authorization_code_grant resource
add snowflake_secret_with_basic_authentication resource
add snowflake_secret_with_generic_string resource
fix parsing oauth_scopes list with comma separated elements

Test Plan

acceptance tests

References

https://docs.snowflake.com/en/sql-reference/sql/create-secret

…aming clash

…validationType

… oauth_scopes to use helper function

…ource # Conflicts: # pkg/acceptance/check_destroy.go # pkg/acceptance/helpers/secret_client.go # pkg/provider/provider.go # pkg/provider/resources/resources.go # pkg/resources/secret_common.go

… dateTime input formats for refresh_token_expiry_time

# Conflicts: # pkg/acceptance/bettertestspoc/assert/objectassert/gen/sdk_object_def.go # pkg/acceptance/helpers/secret_client.go # pkg/sdk/poc/README.md # pkg/sdk/secrets_gen_test.go # pkg/sdk/secrets_impl_gen.go # pkg/sdk/secrets_validations_gen.go # pkg/sdk/testint/secrets_gen_integration_test.go

github-actions · 2024-10-02T15:56:29Z

Integration tests failure for 07dc9e29726c465f1635293ac6b23b7e13fdaf9b

github-actions · 2024-10-03T15:09:47Z

Integration tests failure for 5c6d9ee8b438ea658b934cbd02c5f6bccbc83dcd

github-actions · 2024-10-03T15:33:50Z

Integration tests failure for 805c61e39315df81ff6f61aa9098a2b699ce26bd

github-actions · 2024-10-03T15:35:22Z

Integration tests failure for 044c4846d411d361ab761f43443b18bdaa26af4f

github-actions · 2024-10-03T16:38:04Z

Integration tests failure for 4ae36a482dc1a7721e769128cc8a2ad23dd79efe

github-actions · 2024-10-04T08:06:45Z

Integration tests failure for 4ac851a10ed73c3b48e20d37e3d20f124c75991e

sfc-gh-jmichalak · 2024-10-04T09:56:25Z

pkg/schemas/secret.go

+	},
+	"username": {
+		Type:     schema.TypeString,
+		Optional: true,


Why optional? In other fields too.

Describe for secret in Snowflake leaves some of those fields as "null" since secret can be one of four different types. Thought it needs to be included here as well, but it works as it should after removing the Optional from those fields.

sfc-gh-jmichalak · 2024-10-04T09:57:11Z

pkg/schemas/secret.go

+	}
+	if details.OauthRefreshTokenExpiryTime != nil {
+		s["oauth_refresh_token_expiry_time"] = details.OauthRefreshTokenExpiryTime.String()
+	}


created_on is missing.

added created_on to SecretDescriptionToSchema()

sfc-gh-jmichalak · 2024-10-04T10:00:58Z

pkg/acceptance/helpers/secret_client.go

@@ -71,7 +71,7 @@ func (c *SecretClient) CreateWithBasicAuthenticationFlow(t *testing.T, id sdk.Sc
 func (c *SecretClient) CreateWithGenericString(t *testing.T, id sdk.SchemaObjectIdentifier, secretString string) (*sdk.Secret, func()) {
 	t.Helper()
 	ctx := context.Background()
-	request := sdk.NewCreateWithGenericStringSecretRequest(id, secretString)
+	request := sdk.NewCreateWithGenericStringSecretRequest(id, secretString).WithOrReplace(true)


We should not use OrReplace by default. It should be either an argument, or the function should be called CreateOrReplace... or Recreate...

sfc-gh-jmichalak · 2024-10-04T10:03:54Z

pkg/resources/secret_common.go

+	ShowOutputAttributeName: {
+		Type:        schema.TypeList,
+		Computed:    true,
+		Description: "Outputs the result of `SHOW SECRET` for the given secret.",


should be SHOW SECRETS

has been changed

sfc-gh-jmichalak · 2024-10-04T10:05:33Z

pkg/resources/secret_common.go

+	if err := d.Set("database", secret.DatabaseName); err != nil {
+		return err
+	}
+	if err := d.Set("schema", secret.SchemaName); err != nil {


name, database, and schema should be only in the import function.

sfc-gh-jmichalak · 2024-10-04T10:31:42Z

pkg/resources/secret_with_oauth_authorization_code_grant.go

+		return diag.FromErr(err)
+	}
+
+	if err := client.Secrets.Drop(ctx, sdk.NewDropSecretRequest(id).WithIfExists(*sdk.Bool(true))); err != nil {


It could be simply WithIfExists(true). This applies to other resources.

sfc-gh-jmichalak · 2024-10-04T10:33:23Z

pkg/resources/secret_with_oauth_authorization_code_grant_acceptance_test.go

+	})
+}
+
+func TestAcc_SecretWithAuthorizationCodeGrant_DifferentTimeFormats(t *testing.T) {


sfc-gh-jmichalak · 2024-10-04T10:37:29Z

examples/resources/snowflake_secret_with_client_credentials/resource.tf

+  name               = "EXAMPLE_SECRET"
+  database           = "EXAMPLE_DB"
+  schema             = "EXAMPLE_SCHEMA"
+  api_authentication = "EXAMPLE_SECURITY_INTEGRATION_NAME"


We have the fully_qualified_name field in most of the resources, and we prefer using this instead. Security integrations have simple IDs, but schema-level objects and resources with arguments have complex IDs that are error-prone. Just for the future :)

sfc-gh-jmichalak · 2024-10-04T10:41:50Z

pkg/resources/secret_with_oauth_client_credentials_acceptance_test.go

+						HasDatabaseString(id.DatabaseName()).
+						HasSchemaString(id.SchemaName()).
+						HasApiAuthenticationString(integrationId.Name()),
+					assert.Check(resource.TestCheckResourceAttr(secretModel.ResourceReference(), "oauth_scopes.#", "1")),


You can add a custom method in the resourceassert package - check _ext files. This way we can assert slices, because generator doesn't handle them properly. There, you can compare slices by length, and then check if they have the same elements.

sfc-gh-jmichalak · 2024-10-04T10:42:33Z

pkg/resources/secret_with_oauth_client_credentials_acceptance_test.go

+			},
+			// set oauth_scopes and comment in config
+			{
+				Config: config.FromModel(t, secretModel.


Please also add a test case that checks for external changes - see how we do it in other resources. Add them to all of the secret resources.

sfc-gh-jmichalak · 2024-10-04T10:46:54Z

I forgot to add, but there are missing entries in MIGRATION GUIDE and custom templates in templates/resources/ - please copy one of them and adjust accordingly for each of the resources. We need them to add some custom notes - in this case, we want to note that these resources are v1 release candidates.

sfc-gh-jcieslak · 2024-10-04T09:46:18Z

docs/resources/secret_with_authorization_code_grant.md

+description: |-
+  Secret with OAuth authorization code grant where Secrets Type attribute is set to OAUTH2.
+---
+


The documentation for new resources should contain note at the top that it's v1 candidate. You have to create custom template that's under template/resources/* directory (ask if you need guidance on that; same for other newly added resources).

sfc-gh-jcieslak · 2024-10-04T09:51:02Z

examples/resources/snowflake_secret_with_authorization_code_grant/resource.tf

@@ -0,0 +1,10 @@
+# basic resource


We usually give a basic example that contains only a minimal field resources to create it (e.g., comment is not needed; it's optional) and a complete example with all options set. Of course every resource is different, but this one is pretty straight forward and basic + complete examples would be sufficient. (same applies to other resources).

sfc-gh-jcieslak · 2024-10-04T10:00:26Z

pkg/acceptance/helpers/secret_client.go

@@ -71,7 +71,7 @@ func (c *SecretClient) CreateWithBasicAuthenticationFlow(t *testing.T, id sdk.Sc
 func (c *SecretClient) CreateWithGenericString(t *testing.T, id sdk.SchemaObjectIdentifier, secretString string) (*sdk.Secret, func()) {
 	t.Helper()
 	ctx := context.Background()
-	request := sdk.NewCreateWithGenericStringSecretRequest(id, secretString)
+	request := sdk.NewCreateWithGenericStringSecretRequest(id, secretString).WithOrReplace(true)


Is this needed? I'm guessing it was needed because in some of the tests you're calling Create too many times?

sfc-gh-jcieslak · 2024-10-04T10:04:10Z

pkg/resources/secret_common.go

+	if err := d.Set("database", secret.DatabaseName); err != nil {
+		return err
+	}
+	if err := d.Set("schema", secret.SchemaName); err != nil {


We don't set "name", "database", "schema" in read, we only do it in the import, remove them from here

sfc-gh-jcieslak · 2024-10-04T10:06:11Z

pkg/resources/secret_common.go

+	return nil
+}
+
+func handleSecretUpdate(id sdk.SchemaObjectIdentifier, d *schema.ResourceData) *sdk.AlterSecretRequest {


I would refactor this function to take in *sdk.AlterSecretRequest rather than create it here and either return it or nil. I can imagine there's a bug (or could be) that we would like to use that request and it's nil, because there were no changes done to "comment" field where the request is created.

sfc-gh-jcieslak · 2024-10-04T10:47:49Z

pkg/resources/secret_with_oauth_authorization_code_grant.go

+		return nil, err
+	}
+
+	if err := d.Set("oauth_refresh_token_expiry_time", secretDescription.OauthRefreshTokenExpiryTime.String()); err != nil {


Why this is not read in the Read operation?

sfc-gh-jcieslak · 2024-10-04T10:48:29Z

pkg/resources/secret_with_oauth_authorization_code_grant.go

+	if err := d.Set("oauth_refresh_token_expiry_time", secretDescription.OauthRefreshTokenExpiryTime.String()); err != nil {
+		return nil, err
+	}
+	if err := d.Set("api_authentication", secretDescription.IntegrationName); err != nil {


Import is constructed in a way that always after Import there's Read, so if you read this value in Read operation you can safely remove it.

sfc-gh-jcieslak · 2024-10-04T10:51:00Z

pkg/resources/secret_with_oauth_authorization_code_grant.go

+		return diag.FromErr(err)
+	}
+
+	if err = setStateToValuesFromConfig(d, secretAuthorizationCodeGrantSchema, []string{"oauth_refresh_token_expiry_time"}); err != nil {


I think normal d.Set would be sufficient here (it would change the behaviour, but I think it's better suited for this case.

sfc-gh-jcieslak · 2024-10-04T10:53:03Z

pkg/resources/secret_with_oauth_authorization_code_grant_acceptance_test.go

+					WithComment(newComment),
+				),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{


you can also check action for the whole resource with:

plancheck.ExpectResourceAction(secretModel.ResourceReference(), plancheck.ResourceActionUpdate),

sfc-gh-jcieslak · 2024-10-04T10:56:24Z

pkg/resources/secret_with_oauth_client_credentials.go

+			return diag.Diagnostics{
+				diag.Diagnostic{
+					Severity: diag.Warning,
+					Summary:  "Failed to retrieve secret. Target object not found. Marking the resource as removed.",


It's also to give details here like in the diag below (it's easier to detect for which id this error happened; for us and the user).

sfc-gh-fbudzynski added 30 commits September 24, 2024 10:14

initial secret_def file for sdk

58d5794

naming changes to secret create options

f1488ca

create secrets and alter with two basic tests

752f924

unit tests for alter

d7ae12f

add tests for all four create types

57ca19a

rename previous secret to secretReference in common_types.go due to n…

8d55a63

…aming clash

secrets gen with drop, show and desc

9894799

all unit tests

e4a0827

init for integration tests

3bb8c85

added secret to client.go and created secret_client

4130618

added like and in to showByID for secret

fc85c66

init for integration tests

aa64511

secret_def changes

ab8bee1

tests for two methods of creating secret

3541c70

changed refresh token expiry time from string to time.Time

180a7ce

added remaining integration tests

7a9ad92

linter

0e9cf94

linting

deadc65

removing comment

235fbe5

mid review commit before generating assertions

73eeb63

rebase conflict resolved

bd0b64f

tests pass after changing to []string for oauthScopes

2050b85

readded unit tests

59c7b14

added CreateApiAuthenticationWithRequest to security integration client

df7a8ff

changes to secret after review, all tests pass

eed310a

added assert for details

af2e030

linter hints applied

4ccc4e7

updated generator README with validation issue for ConflictingFields …

34983af

…validationType

updated after re-review

bdf2585

test changed to use errMoreThanOneOf

7791f33

sfc-gh-fbudzynski added 12 commits September 30, 2024 14:50

Secret with authorization code acceptance test, diff suppress

46e2181

helper function for snowflake datetime for secret

c3f3ea7

check destroy and few cleanups

91269d6

adding secret with basic authentication flow

b91b97c

checking secret with optional oauth_scopes

8de6e84

secret basic authentication flow acceptance tests and fix for parsing…

c64a1ed

… oauth_scopes to use helper function

Merge remote-tracking branch 'origin/secret-resource' into secret-res…

ccdf9f8

…ource # Conflicts: # pkg/acceptance/check_destroy.go # pkg/acceptance/helpers/secret_client.go # pkg/provider/provider.go # pkg/provider/resources/resources.go # pkg/resources/secret_common.go

removed unwanted duplicats after merge

fde6908

All secret flows with acceptance tests and change to handle different…

a147b99

… dateTime input formats for refresh_token_expiry_time

linter pre-push changes

356d98c

generated documentation for each secret flow

83ccafc

sfc-gh-fbudzynski changed the title ~~Secret resource~~ feat: Secret resource Oct 2, 2024

sfc-gh-fbudzynski added 6 commits October 3, 2024 14:36

delete unused _ext assert

de99b68

self-review cleanup

5c6d9ee

docs regenerate

805c61e

example resource naming fix for secret with generic_string

044c484

docs fixes

bc43a72

make docs-check error fix

4ae36a4

Merge branch 'main' into secret-resource

4ac851a

sfc-gh-fbudzynski marked this pull request as ready for review October 4, 2024 06:51

sfc-gh-jmichalak requested changes Oct 4, 2024

View reviewed changes

sfc-gh-jcieslak requested changes Oct 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Secret resource #3110

feat: Secret resource #3110

sfc-gh-fbudzynski commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 4, 2024

sfc-gh-jmichalak Oct 4, 2024

sfc-gh-fbudzynski Oct 4, 2024

sfc-gh-jmichalak Oct 4, 2024

sfc-gh-fbudzynski Oct 4, 2024

sfc-gh-jmichalak Oct 4, 2024

sfc-gh-jmichalak Oct 4, 2024

sfc-gh-fbudzynski Oct 4, 2024

sfc-gh-jmichalak Oct 4, 2024

sfc-gh-fbudzynski Oct 4, 2024

sfc-gh-jmichalak Oct 4, 2024

sfc-gh-jmichalak Oct 4, 2024

sfc-gh-jmichalak Oct 4, 2024

sfc-gh-jmichalak Oct 4, 2024

sfc-gh-jmichalak Oct 4, 2024

sfc-gh-jmichalak commented Oct 4, 2024

sfc-gh-jcieslak Oct 4, 2024

sfc-gh-jcieslak Oct 4, 2024

sfc-gh-jcieslak Oct 4, 2024

sfc-gh-jcieslak Oct 4, 2024

sfc-gh-jcieslak Oct 4, 2024

sfc-gh-jcieslak Oct 4, 2024

sfc-gh-jcieslak Oct 4, 2024

sfc-gh-jcieslak Oct 4, 2024

sfc-gh-jcieslak Oct 4, 2024

sfc-gh-jcieslak Oct 4, 2024

feat: Secret resource #3110

Are you sure you want to change the base?

feat: Secret resource #3110

Conversation

sfc-gh-fbudzynski commented Oct 2, 2024 • edited Loading

Changes

Test Plan

References

github-actions bot commented Oct 2, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 4, 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

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

sfc-gh-jmichalak commented Oct 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-fbudzynski commented Oct 2, 2024 •

edited

Loading