feat: add resource snowflake_user_password_policy_attachment (#2162)

[raulbonet]

Description

ref #2162
This PR creates the new resource snowflake_user_password_policy_attachment. The implementation mimics the one of its brother, snowflake_account_password_policy_attachment

[sfc-gh-asawicki]

Hey @raulbonet. Thanks for the PR.

To merge it, we require a few more things:

• (easy) generate the documentation of the new resource (run make dev-setup && make docs);
• (medium) add test cases or steps to the prepared test which shows update and delete behavior;
• (medium) validation in tests that the password policy is assigned to the user (consult getRowAccessPolicyFor and policyReference in helpers_test.go);
• (medium) implement read and import methods.

The reason for that is that publishing a partially implemented/not thoroughly tested resource will result in more issues being created. We want to avoid that.

Would you be willing to introduce these changes? Alternatively, we can merge it as a separate branch and add the missing parts ourselves (but probably not earlier than Feb-Mar).

cc: @sfc-gh-swinkler @sfc-gh-jcieslak

[raulbonet]

Hello @sfc-gh-asawicki

Thanks for coming back to me. About your comments, I am happy to do all of this myself, but I want to make sure we are in the same page. As I was mentioning in the OP:

1. The "brother" of this resource, snowflake_account_password_policy_attachment, does not implement (properly) the Read() method either (it only reads if the network policy exists, but not if it is attached). Is it a bug?

2. I think there are several resources that do not implement the Read functionality (like a lot of the grants I believe). I thought precisely that you were merging stuff in iterations, but if it is not the case from now on, happy to fully develop it.

3. The Read() method is not implemented because the new sdk lacks the implementation to read from POLICY REFERENCES (I think it is possibly to do it with the interfaces in snowflake, but I guess from the code that you are deprecating this). I am happy to implement it myself, but let's make sure we are in the same page:

   3.1. You would like me to create a a new interface in the sdk, let's say policy_references.go, which among other things, method mimics this: https://docs.snowflake.com/en/sql-reference/functions/policy_references ?

   3.2. If so, would you like to:
   a) Create one single interface for all kinds of policy references (masking, session, password, row access)
   b) Create a specific interface for password (so, policy_references_password.go)

[sfc-gh-asawicki]

Hey @raulbonet. That's great to hear that! I've included my answers below.

1. The "brother" of this resource, snowflake_account_password_policy_attachment, does not implement (properly) the Read() method either (it only reads if the network policy exists, but not if it is attached). Is it a bug?

Maybe not a bug, but an imperfection. :) It should be implemented too, but this is low on our roadmap. As you may have noticed, the team changed in the past few months. The resource was introduced earlier in a rushed way. We follow a bit different development cycle now. In other words, we try to avoid introducing missing/untested functionalities and are slowly correcting the buggy/incomplete ones.

You can add it too, if you want. But please do it as a separate PR (we prefer PRs targeting one issue only).

2. I think there are several resources that do not implement the Read functionality (like a lot of the grants I believe). I thought precisely that you were merging stuff in iterations, but if it is not the case from now on, happy to fully develop it.

We are merging stuff in iterations, but only in certain cases. One example would be when we introduce a new resource in the SDK. It does not change the provider's behavior and can be later used in a separate PR to migrate the resource from the old snowflake one to the new SDK client.

Here, the situation is different. An entirely new resource would be introduced, so people would start using it. And that may result in more issues being reported. We want to limit possible problems by correctly preparing the resource.

3. The Read() method is not implemented because the new sdk lacks the implementation to read from POLICY REFERENCES (I think it is possibly to do it with the interfaces in snowflake, but I guess from the code that you are deprecating this). I am happy to implement it myself, but let's make sure we are in the same page:

It is not implemented in the SDK but was already used for testing. It is here with a TODO comment to make it a not-test-only resource:

terraform-provider-snowflake/pkg/sdk/testint/helpers_test.go

Line 773 in be01d5f

func getRowAccessPolicyFor(t *testing.T, client *sdk.Client, id sdk.SchemaObjectIdentifier, objectType sdk.ObjectType) (*policyReference, error) {

.

3.1. You would like me to create a a new interface in the sdk, let's say policy_references.go, which among other things, method mimics this: https://docs.snowflake.com/en/sql-reference/functions/policy_references ?

Yes. But let's limit it just to POLICY REFERENCES (the ENTITY syntax; I think we don't need the POLICY NAME one).

3.2. If so, would you like to:
a) Create one single interface for all kinds of policy references (masking, session, password, row access)
b) Create a specific interface for password (so, policy_references_password.go)

For now, you can go with 3.2.a -> e.g. PolicyReferences#GetForEntity. You can either reuse the existing implementation (we have one like that for system functions:

terraform-provider-snowflake/pkg/sdk/system_functions.go

Line 22 in be01d5f

sql := fmt.Sprintf(`SELECT SYSTEM$GET_TAG('%s', '%s', '%v') AS "TAG"`, tagID.FullyQualifiedName(), objectID.FullyQualifiedName(), objectType)

) but preferably we would like to stick with the syntax using opts and request (like CreateDatabaseRoleRequest and createDatabaseRoleOptions). We will accept either way.

We tend to add unit tests (like TestDatabaseRoleCreate) and validations (like func (opts *createDatabaseRoleOptions) validate()). Also, we add integration tests (like in database_role_integration_test.go) that validate the SDK in separation from Terraform (one day, we will extract the SDK as a separate project).

If you have any more questions (or more questions will appear during the implementation), feel free to ask them!

And so you know: we are aware that we lack clear contributing guidelines. We are currently working on transparency, so we should have an updated contributing guide ready in the upcoming weeks.

[sfc-gh-asawicki]

@raulbonet, thanks for all the changes you made after our reviews! It's very helpful for us to receive help with the implementation.

Before we proceed, please answer my comment: #2307 (comment), the part about unit and integration tests. We can write or help you write them, but we need a consensus here because we can only merge the change with them.

[raulbonet]

Hello @sfc-gh-asawicki
Sorry, yesterday night I made the changes and did not answer your message.

1. About the computed field issue: I think I will create a parallel branch and re-test the behaviour because I think that the behaviour was incorrect, but I might be wrong: even when you changed the database or schema of the Password Policy (which triggers a replacement of the PP), this did not in turn trigger a replacement of the attachment, but I am going to double check.

2. About unit test: I can add them.

3. About integration tests: we would love to collaborate but this is a bit more delicate: unfortunately, I am seeing that the integration tests (and acceptance tests) might leave some "resource traces" in our Snowflake account (warehouses, users, etc.), particularly if they fail, so we do not feel comfortable running these in our Snowflake account. We requested an extra Snowflake Account to our Account manager but we did not get an answer; until we have this sorted out we prefer not to run these. Would you mind chiming in for the integration tests?

[raulbonet]

About the delimiters: the sqlBuilder transforms the " into \" . I guess the problem comes from the workaround we used because the builder does not support both single quotations and double quotations at the same time: so, the sqlBuilder interprets the quotes " as a string, and prefixes them with a backslash.

I solved it with this workaround you see:

strings.ReplaceAll(userName.FullyQualifiedName(), `"`, `\"`)

[sfc-gh-asawicki]

Seems fine, we will check the statement execution with the integration tests and change the sqlbuilder behavior or opts config if needed.

[gitguardian]

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
-		Generic Password	25e6a2c	pkg/resources/managed_account_test.go	View secret
-		Generic Password	25e6a2c	pkg/resources/managed_account_test.go	View secret

🛠 Guidelines to remediate hardcoded secrets

1. Understand the implications of revoking this secret by investigating where it is used in your code.
2. Replace and store your secrets safely. Learn here the best practices.
3. Revoke and rotate these secrets.
4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

• following these best practices for managing and storing secrets including API keys and other credentials
• install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

---

<sup>🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!</sup>

[sfc-gh-asawicki]

/ok-to-test sha=25e6a2c76a138bc3d4b58d6d959d713192d1a7d3

[sfc-gh-asawicki]

@raulbonet Thanks for the contribution one more time!

We decided to merge it and add the integration tests on our side. If we uncover any problems with the generated statements we will just fix them. Merging without them should not affect anyone because we plan the next release for Wednesday/Thursday next week. We may also adjust the resource schema to what we discussed (single fully qualified id instead of 3 parts but it is yet to be decided).

[github-actions]

Integration tests failure for 25e6a2c76a138bc3d4b58d6d959d713192d1a7d3