Add authentication policy resource #3098

Relativity74205 · 2024-09-21T19:45:43Z

Added the following resources:

authentication_policy
account_authentication_policy_attachment
user_authentication_policy_attachment

Test Plan

acceptance tests (have been mostly added; could not be tested locally due to difficulties with acceptance test setup

manual tests

References

[Feature]: Add support for Authentication Policies #2880

Relativity74205 · 2024-09-21T19:57:09Z

@sfc-gh-jcieslak
Here are some details about my problems with running the acceptance tests locally:

For starters I had to remove the following code from test setup, why do I need a SCIM Provisioner Role for running the acceptance tests?

	if err := helpers.EnsureScimProvisionerRolesExist(atc.client, ctx); err != nil {
		t.Fatal(err)
	}
	
	if err := helpers.EnsureScimProvisionerRolesExist(atc.secondaryClient, ctx); err != nil {
		t.Fatal(err)
	}

In addition, I noticed after some testing, that I had 10+ warehouses running in the snowflake account of my company, which I use for testing. Althorugh all warehouses were of size XS, the suspend time was set to 600 seconds. At this point I stopped setting up my test setup, because there seems to be something very odd. Perhaps I am making something wrong?

csp33 · 2024-09-23T06:25:20Z

Hi! Thanks for this PR, it is a really important one for my team :)
In v0.95, many user parameters (such as +network_policy) were moved to the snowflake_user object (source). I wonder if this user_authentication_policy_attachment should also be moved (or duplicated) in snowflake_user.authentication_policy

sfc-gh-jmichalak · 2024-09-23T08:02:33Z

Hi @Relativity74205 @csp33 👋

Thanks a lot for your contribution :) We'll probably review this PR this week.

The code you quoted checks for some roles that are required for other acceptance tests to work. You can disable them for your tests (remove the code).

About these warehouses, we have sweepers which should remove stale objects. You can enable them with TEST_SF_TF_ENABLE_SWEEP=1.

I think this should be added as a part of the snowflake_user resource. WDYT @sfc-gh-jcieslak @sfc-gh-asawicki ?

sfc-gh-jcieslak · 2024-09-23T08:04:01Z

Hey
@Relativity74205 Thanks for the pr! As far as I remember, SCIM provisioner roles are only needed for security integration tests, so they're not needed for your tests. Regarding warehouses, I think the issue is that in our CI, we are running sweepers (sweepers_test.go) that remove those predefined warehouses/databases/schemas (those objects have a common prefix that allows us to differentiate between manually created objects and the one produced by acceptance test). You can specify your own suffix by specifying the TEST_SF_TF_TEST_OBJECT_SUFFIX env variable (you can see how the names will be generated in acceptance/testing.go). That way you can track created objects and drop them after your testing is done. It's not ideal, and I think we could improve this so that it will be easier for ppl that want to contribute, but that's what we have to work with right now. I'll try to review this today/tomorrow. Important note to what @sfc-gh-jmichalak proposed: if you have other objects and you are sharing the account with someone else, I wouldn't consider sweeping because it's very destructive. After setting TEST_SF_TF_TEST_OBJECT_SUFFIX our pre-test function will only create warehouse/database/schema once and every other test won't create anything because the suffix will be the same and objects will be already present.

@csp33 We have a ticket to add policies to user resources; it's assigned to @sfc-gh-asawicki. He's on vacation right now, but I'm guessing that would be one of his priorities when he'll be back (this week).

pkg/resources/account_password_policy_attachment_acceptance_test.go

sfc-gh-jmichalak · 2024-09-24T08:20:13Z

pkg/provider/provider.go

-		"snowflake_account_password_policy_attachment": resources.AccountPasswordPolicyAttachment(),
-		"snowflake_account_parameter":                  resources.AccountParameter(),
-		"snowflake_alert":                              resources.Alert(),
+		"snowflake_account": resources.Account(),


Please format your code with make fmt.

That is how it is auto formatted for some reason...

That's interesting, but we can fix this on our side. Let's leave this unresolved.

sfc-gh-jmichalak · 2024-09-24T08:21:03Z

pkg/provider/provider.go

-		"snowflake_account_parameter":                  resources.AccountParameter(),
-		"snowflake_alert":                              resources.Alert(),
+		"snowflake_account": resources.Account(),
+		"snowflake_account_authentication_policy_attachment":                     resources.AccountAuthenticationPolicyAttachment(),


I don't think we need these attachment resources - we should attach directly in users and accounts resources.

You mean similar to the network policy attachment? I have taken the password policy resource and its attachment as an example.

Hmm, I thought that they could be specified in the user's resource.

What way should we take here? @sfc-gh-jcieslak @sfc-gh-asawicki

@sfc-gh-jmichalak How shall I procede here?

pkg/resources/testdata/TestAcc_AuthenticationPolicy/test.tf

pkg/resources/authentication_policy.go

sfc-gh-jmichalak · 2024-09-24T09:09:56Z

pkg/resources/authentication_policy_acceptance_test.go

+)
+
+func TestAcc_AuthenticationPolicy(t *testing.T) {
+	accName := acc.TestClient().Ids.Alpha()


We need more tests, like

basic flow

create without optional

set optional fields

import

handle external change (one field is enough)

unset optional fields

import

renaming
Please take a look at row_access_policy_acceptance_test.go - specifically TestAcc_RowAccessPolicy and TestAcc_RowAccessPolicy_Rename.

We use custom generated models in there, but it can be tricky to set up, and I think you can use TF files + config map.

@sfc-gh-jmichalak
I have not fixed my local test setup and can now finalize the tests. However, one question, how to do "handle external change (one field is enough)"?

pkg/resources/authentication_policy.go

csp33 · 2024-10-03T09:50:25Z

hi @sfc-gh-jmichalak @sfc-gh-jcieslak

Is there progress related to this PR?
Thanks! 😄

sfc-gh-jcieslak · 2024-10-03T10:54:00Z

Hey @csp33
That's more of a question for @Relativity74205 as this is his pull request. After applying essential changes we'll be able to merge it and make some of the adjustments ourselves to apply the standards (or at least some of them) we defined for v1 resource refactoring.

Relativity74205 · 2024-10-03T13:24:54Z

I have started to look on the PR comments, I guess I can fix most of the stuff today/tomorrow. However, I need to get the testsuite running, before I start adding some additional tests. In contrast to the past, I had some major issues last week and after I noticed 15-20 warehouses running in parallel in th snowflake account, I stopped testing.

I will have a look on the hints given above by @sfc-gh-jcieslak and @sfc-gh-jmichalak , perhaps I will be more successful. However, I would suggest to make the usability of the test suite a little more user friendly.

Relativity74205 · 2024-10-03T21:25:22Z

I think I have resolved most issues, however, some are still open, because I need some clarifications. And I need to finish to setup the test suite and finish the tests after that.

examples/resources/snowflake_authentication_policy/import.sh

examples/resources/snowflake_authentication_policy/resource.tf

sfc-gh-jmichalak · 2024-10-04T08:23:53Z

pkg/resources/authentication_policy.go

+		Type: schema.TypeSet,
+		Elem: &schema.Schema{
+			Type:             schema.TypeString,
+			ValidateDiagFunc: StringInSlice([]string{"ALL", "SAML", "PASSWORD", "OAUTH", "KEYPAIR"}, false),


Yes, please check out warehouse enums in sdk - To... functions.

sfc-gh-jmichalak · 2024-10-04T08:30:34Z

pkg/resources/authentication_policy.go

+			authenticationMethods := expandStringList(v.(*schema.Set).List())
+			authenticationMethodsValues := make([]sdk.AuthenticationMethods, len(authenticationMethods))
+			for i, v := range authenticationMethods {
+				authenticationMethodsValues[i] = sdk.AuthenticationMethods{Method: sdk.AuthenticationMethodsOption(v)}


After implementing To... enum validators, they should be used in all places when we convert from strings in user inputs to SDK fields.

done, please check

sfc-gh-jmichalak · 2024-10-04T08:33:12Z

pkg/resources/authentication_policy.go

-			mfaAuthenticationMethodsValues[i] = sdk.MfaAuthenticationMethods{Method: sdk.MfaAuthenticationMethodsOption(v)}
+	// change to authentication methods
+	if d.HasChange("authentication_methods") {
+		if v, ok := d.GetOk("authentication_methods"); ok {


When this list is empty in the new config, this block won't execute. I think the check if len(authenticationMethodsValues) == 0 should be in else block for this if. Same for other fields.

In general, we do it like:

if v, ok := d.GetOk("authentication_methods"); ok { // handle changed, non-empty field } else { // handle empty field }

pkg/resources/authentication_policy.go

sfc-gh-jcieslak · 2024-10-04T11:42:34Z

Thank you for your feedback @Relativity74205 I added you suggestion to our tasks about documentation improvement to clearly state how to use the testing suite and also added suggestion in other tasks to make it more friendly for newcomers.

…ing to account user missing)

Relativity74205 · 2024-10-18T07:02:26Z

@sfc-gh-jcieslak @sfc-gh-jmichalak
I noticed a "bigger" problem, when I was testing the authentication policy manually. The problem are the set-parameters, because they have defaults. When running a terraform plan on an authentication policy, where e.g. authentication_methods was not set, I get

  # snowflake_authentication_policy.name will be updated in-place
  ~ resource "snowflake_authentication_policy" "name" {
      ~ authentication_methods     = [
          - "ALL",
        ]
        ....
    }

where ["ALL"] is the Snowflake default for this parameter.

However, setting the Default property in the schema definition of the resource, does not work, because of authentication_methods: Default is not valid for lists or sets. Therefore, what is the best way to resolve this? The only thing, which comes to my mind, is building a manual check inside of the ReadContextAuthenticationPolicy function.

PS: It works now, after I have added a custom check to ReadContextAuthenticationPolicy, it looks in general like this:

	authenticationPolicyDescriptions, err := client.AuthenticationPolicies.Describe(ctx, id)
	if err != nil {
		return diag.FromErr(err)
	}

	authenticationMethodsIs := make([]string, 0)
	if authenticationMethodsProperty, err := collections.FindFirst(authenticationPolicyDescriptions, func(prop sdk.AuthenticationPolicyDescription) bool { return prop.Property == "AUTHENTICATION_METHODS" }); err == nil {
		authenticationMethodsIs = append(authenticationMethodsIs, sdk.ParseCommaSeparatedStringArray(authenticationMethodsProperty.Value, false)...)
	}
	authenticationMethodsShould := d.Get("authentication_methods").(*schema.Set).List()
	if stringSlicesEqual(authenticationMethodsIs, []string{"ALL"}) && len(authenticationMethodsShould) == 0 {
		authenticationMethodsIs = []string{}
	}
	if err = d.Set("authentication_methods", authenticationMethodsIs); err != nil {
		return diag.FromErr(err)
	}

PS2: The unwanted consequence of the "hack" in ReadContext is that also the Import function needs a "hack":

	authenticationPolicyDescriptions, err := client.AuthenticationPolicies.Describe(ctx, id)
	securityIntegrationsIs := make([]string, 0)
	if securityIntegrationsProperty, err := collections.FindFirst(authenticationPolicyDescriptions, func(prop sdk.AuthenticationPolicyDescription) bool { return prop.Property == "SECURITY_INTEGRATIONS" }); err == nil {
		securityIntegrationsIs = append(securityIntegrationsIs, sdk.ParseCommaSeparatedStringArray(securityIntegrationsProperty.Value, false)...)
	}
	if err = d.Set("security_integrations", securityIntegrationsIs); err != nil {
		return nil, err
	}

PS3: I pushed a version, which seems to be working fine, however, I don't like it with the "hacks". Is there a better solution?

Actually, one other solution comes to my mind, which would prevent this entire problem: Removing the optional attribute of these parameters. When the list parameters are required, then the default check isn't needed. However, this wouldn't be intuitive for the user as these parameters are optional in Snowflake.

sfc-gh-jcieslak · 2024-10-18T14:44:25Z

Hey @Relativity74205 👋
We'll have some time on Monday to review the change again. Regarding authentication_methods, Terraform SDKv2 is pretty limited when it comes to ignoring such changes. We had to come up with pretty tricky solutions to handle some of the cases. This case looks pretty similar to the warehouse_type field in the warehouse resource. The solution is to not set this value in Read at all (only set it in Import). The Terraform should handle the changes correctly after that. You can try that or leave it as is, and we'll take a closer look at it after we merge it (there may be some additional difficulties with handling that, because it's a List/Set and not a primitive type which are usually easier to handle).

Relativity74205 mentioned this pull request Sep 21, 2024

[Feature]: Add support for Authentication Policies #2880

Open

1 task

sfc-gh-jmichalak requested changes Sep 24, 2024

View reviewed changes

sfc-gh-jcieslak reviewed Sep 25, 2024

View reviewed changes

pkg/resources/authentication_policy.go Outdated Show resolved Hide resolved

pkg/resources/authentication_policy.go Outdated Show resolved Hide resolved

pkg/resources/authentication_policy.go Outdated Show resolved Hide resolved

sfc-gh-jmichalak requested changes Oct 4, 2024

View reviewed changes

AS-auxmoney added 8 commits October 17, 2024 22:05

feat: added authentication policy resource (WIP, test missing and add…

e65a335

…ing to account user missing)

feat: added authentication_policy attachments and added first tests

be647f8

feat: finalized authentication_policy incl. attachments

930ec4d

test: fix

56e0b51

fix: CR

8bb9ccb

fix: a lot of fixed/refactorings from CR

dfe03ff

docs: added example

1ad1461

fix: added description

a2f467a

Relativity74205 force-pushed the add-authentication-policies branch from bc8534a to a2f467a Compare October 17, 2024 20:06

AS-auxmoney added 3 commits October 17, 2024 22:15

fix: small PR fixes

f997314

fix: added To*Option functions in authentication_policies SDK

efd6d7c

refactor: PR comment

71bb605

AS-auxmoney added 3 commits October 18, 2024 11:10

feat: added show and describe schemas

d29425d

fix: fixed read with list defaults

98e47ec

fix: fixed imports

384b48b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authentication policy resource #3098

Add authentication policy resource #3098

Relativity74205 commented Sep 21, 2024

Relativity74205 commented Sep 21, 2024

csp33 commented Sep 23, 2024

sfc-gh-jmichalak commented Sep 23, 2024 •

edited

Loading

sfc-gh-jcieslak commented Sep 23, 2024 •

edited

Loading

sfc-gh-jmichalak Sep 24, 2024

Relativity74205 Oct 3, 2024

sfc-gh-jmichalak Oct 4, 2024 •

edited

Loading

sfc-gh-jmichalak Sep 24, 2024

Relativity74205 Oct 3, 2024

sfc-gh-jmichalak Oct 4, 2024

Relativity74205 Oct 17, 2024

sfc-gh-jmichalak Sep 24, 2024

Relativity74205 Oct 18, 2024

csp33 commented Oct 3, 2024

sfc-gh-jcieslak commented Oct 3, 2024

Relativity74205 commented Oct 3, 2024

Relativity74205 commented Oct 3, 2024

sfc-gh-jmichalak Oct 4, 2024

sfc-gh-jmichalak Oct 4, 2024

Relativity74205 Oct 17, 2024

sfc-gh-jmichalak Oct 4, 2024

Relativity74205 Oct 17, 2024 •

edited

Loading

sfc-gh-jcieslak commented Oct 4, 2024

Relativity74205 commented Oct 18, 2024 •

edited

Loading

sfc-gh-jcieslak commented Oct 18, 2024

Add authentication policy resource #3098

Are you sure you want to change the base?

Add authentication policy resource #3098

Conversation

Relativity74205 commented Sep 21, 2024

Test Plan

References

Relativity74205 commented Sep 21, 2024

csp33 commented Sep 23, 2024

sfc-gh-jmichalak commented Sep 23, 2024 • edited Loading

sfc-gh-jcieslak commented Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

csp33 commented Oct 3, 2024

sfc-gh-jcieslak commented Oct 3, 2024

Relativity74205 commented Oct 3, 2024

Relativity74205 commented Oct 3, 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

Relativity74205 Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

sfc-gh-jcieslak commented Oct 4, 2024

Relativity74205 commented Oct 18, 2024 • edited Loading

sfc-gh-jcieslak commented Oct 18, 2024

sfc-gh-jmichalak commented Sep 23, 2024 •

edited

Loading

sfc-gh-jcieslak commented Sep 23, 2024 •

edited

Loading

sfc-gh-jmichalak Oct 4, 2024 •

edited

Loading

Relativity74205 Oct 17, 2024 •

edited

Loading

Relativity74205 commented Oct 18, 2024 •

edited

Loading