Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add authentication policy resource #3098

Merged

Conversation

Relativity74205
Copy link
Contributor

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

@Relativity74205
Copy link
Contributor Author

@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
Copy link

csp33 commented Sep 23, 2024

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
Copy link
Collaborator

sfc-gh-jmichalak commented Sep 23, 2024

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
Copy link
Collaborator

sfc-gh-jcieslak commented Sep 23, 2024

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).

"snowflake_account_password_policy_attachment": resources.AccountPasswordPolicyAttachment(),
"snowflake_account_parameter": resources.AccountParameter(),
"snowflake_alert": resources.Alert(),
"snowflake_account": resources.Account(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please format your code with make fmt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@sfc-gh-jmichalak sfc-gh-jmichalak Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pkg/provider/provider.go Show resolved Hide resolved
pkg/resources/authentication_policy.go Outdated Show resolved Hide resolved
pkg/resources/authentication_policy.go Outdated Show resolved Hide resolved
if err = d.Set("database", authenticationPolicy.DatabaseName); err != nil {
return diag.FromErr(err)
}
if err = d.Set("schema", authenticationPolicy.SchemaName); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 3 fields should be set in Import function - check resource_monitor.go

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be only set in the import function, we don't set those fields in the newly refactored resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created an ImportAuthenticationPolicy function and moved the 3 fields to this function. However, I don't really understand what I have done there/this design change overall, therefore please check if this is correct this way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(resolve when read) We made a decision not to read "database", "schema", and "name", because:

  1. When changed in the configuration, the Terraform detects this change anyway (and sets it in the state).
  2. When reading from Snowflake, it may return different format causing infinite plan on e.g. quotes or casing of the identifier that we would like to avoid. This would be extra unfortunate with the case of database and schema (and sometimes name) which are marked here as ForceNew, causing infinite create-destroy loop.

+ Probably other cases, but ultimately, it's not needed for the provider to work and we're better off not setting them.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need more tests, like

  • basic flow
    1. create without optional
    2. set optional fields
    3. import
    4. handle external change (one field is enough)
    5. unset optional fields
    6. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, you have use PreConfig (couldn't find a basic usage, but you can search by PreConfig and see how it's used) in the second step (or somewhere later on) and use raw SQL client to change the state of an object "by hand". Then, within the same test step you ensure that the values went back to the ones that are set in the Terraform configuration. For example, you have set comment in the Terraform configuration to "foo", then in the second step in PreConfig, you use acc.TestClient().AuthPolicy.Alter(...) to change this comment to "bar" and in the Check you assert comment to contain "foo", because we expect the provider to override incorrect value.

pkg/resources/authentication_policy.go Show resolved Hide resolved
pkg/resources/authentication_policy.go Outdated Show resolved Hide resolved
pkg/resources/authentication_policy.go Outdated Show resolved Hide resolved
if err = d.Set("database", authenticationPolicy.DatabaseName); err != nil {
return diag.FromErr(err)
}
if err = d.Set("schema", authenticationPolicy.SchemaName); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be only set in the import function, we don't set those fields in the newly refactored resources.

@csp33
Copy link

csp33 commented Oct 3, 2024

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

Is there progress related to this PR?
Thanks! 😄

@sfc-gh-jcieslak
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

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 Show resolved Hide resolved
pkg/resources/authentication_policy.go Outdated Show resolved Hide resolved
pkg/resources/authentication_policy.go Show resolved Hide resolved
@sfc-gh-jcieslak
Copy link
Collaborator

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.

@Relativity74205
Copy link
Contributor Author

Relativity74205 commented Oct 18, 2024

@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
Copy link
Collaborator

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).

Copy link
Collaborator

@sfc-gh-jcieslak sfc-gh-jcieslak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing the comments.
For the future commits/prs, please try to use merge instead of rebase. GitHub doesn't handle rebases well and it's harder to see what changed since last review.

if err = d.Set("database", authenticationPolicy.DatabaseName); err != nil {
return diag.FromErr(err)
}
if err = d.Set("schema", authenticationPolicy.SchemaName); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(resolve when read) We made a decision not to read "database", "schema", and "name", because:

  1. When changed in the configuration, the Terraform detects this change anyway (and sets it in the state).
  2. When reading from Snowflake, it may return different format causing infinite plan on e.g. quotes or casing of the identifier that we would like to avoid. This would be extra unfortunate with the case of database and schema (and sometimes name) which are marked here as ForceNew, causing infinite create-destroy loop.

+ Probably other cases, but ultimately, it's not needed for the provider to work and we're better off not setting them.

)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, you have use PreConfig (couldn't find a basic usage, but you can search by PreConfig and see how it's used) in the second step (or somewhere later on) and use raw SQL client to change the state of an object "by hand". Then, within the same test step you ensure that the values went back to the ones that are set in the Terraform configuration. For example, you have set comment in the Terraform configuration to "foo", then in the second step in PreConfig, you use acc.TestClient().AuthPolicy.Alter(...) to change this comment to "bar" and in the Check you assert comment to contain "foo", because we expect the provider to override incorrect value.

Copy link
Collaborator

@sfc-gh-jmichalak sfc-gh-jmichalak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the follow-up! Regarding the comments I've added, I think we can handle them on our side cc @sfc-gh-asawicki @sfc-gh-jcieslak

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
Description: "Determines whether a user must enroll in multi-factor authentication. Allowed values are REQUIRED and OPTIONAL. When REQUIRED is specified, Enforces users to enroll in MFA. If this value is used, then the CLIENT_TYPES parameter must include SNOWFLAKE_UI, because Snowsight is the only place users can enroll in multi-factor authentication (MFA).",
ValidateDiagFunc: sdkValidation(sdk.ToMfaEnrollmentOption),
DiffSuppressFunc: SuppressIfAny(NormalizeAndCompare(sdk.ToMfaEnrollmentOption)),
Default: "OPTIONAL",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not use defaults in the schema. But I think we can change this on our side because it's a bit more complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will leave it as it is

pkg/resources/authentication_policy.go Outdated Show resolved Hide resolved
pkg/provider/provider.go Show resolved Hide resolved
Description: "Specifies the authentication policy to use for the current account. To set the authentication policy of a different account, use a provider alias.",

Create: CreateAccountAuthenticationPolicyAttachment,
Read: ReadAccountAuthenticationPolicyAttachment,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also support Update here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure, the only parameter authentication_policy has ForceNew enabled and with the other attachment resources it is the same.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was not clear: Can we have authentication_policy without ForceNew and handle Update in such a function? I think it should be possible.

Copy link
Contributor Author

@Relativity74205 Relativity74205 Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a good example to bring up a point. I wouldn't to it at this moment, because it would introduce another inconsistency into the codebase and there are already tons of them, which becomes extremely frustrating for me as someone, which contributes only from time to time. When I run into a problem or am implementing something, I usually look at examples at another place in the codebase. However, for every given thing, there are at least 2 (usually more) options how to do it.
And at least have of the comments in this PR were "we don't do it in this way any more" or something similar, which becomes very frustrating.
Another example I am currently struggling with, is how acceptance_test are done. If I am not mistaken, there are at least three different ways. So which one shall I use, I guess is should be Better tests poc as with the warehouse resource (here I have the problem, that the readme has some errors and the generator makes changes to existing generated files)? Or shall I take another approach?

Regarding your original request: I would do it in another PR and make changes to all attachment resources at the same time for consistency.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

First of all, we really appreciate community contributions. It's great to see users' interest in this project.

The project was started by the community and taken over by Snowflake after a while. So, we expect that there will be some differences in the code. During our work before v1, we came up with a few different ideas, and we want to choose the best one ultimately, but we can't improve every resource at once.

To make contributing more straightforward, we have provided a contribution guide, and if you have any questions regarding this project, don't hesitate to ask us :)

The generator you mentioned is a PoC for speeding up writing tests. It is incomplete and has some limitations, as stated in the generator's README.

Regarding the Update thing, as I stated above, we can handle it on our side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfc-gh-jmichalak Sure, I am following the project for some while now while I made some small contributions. And of course, I can understand your position, that you are testing different ideas to find a way to make this project more maintainable in the future and I can perfectly understand why this project is in this state. However, this current state is quite difficult/frustrating for someone like me, which works in this project only 2-3 a year for some weeks and has to understand, what the current best-practice is and where to find correct examples how to implement certain things.
And yes, I have read the contributing guide, however, in this current state it is only partially helpful.

And btw, now from the perspective of the user: Consistent behaviour across similar resources is something what a user wishes a lot. Therefore I wouldn't recommend to add the update behaviour only to the authentication_policy_attachment, but to do it consistent for all attachments.

And thanks for taking over the rest of the work on this resource. :)

@Relativity74205
Copy link
Contributor Author

Thanks for the follow-up! Regarding the comments I've added, I think we can handle them on our side cc @sfc-gh-asawicki @sfc-gh-jcieslak

@sfc-gh-jmichalak
I have fixed some of the small stuff. Then whats left for me, are finalizing the tests, I will do this probably this evening. And I will have a look on #3098 (comment), however, I will leave it like this, if I don't find a better solution quickly.

@sfc-gh-jmichalak sfc-gh-jmichalak changed the title Add authentication policy resource feat: Add authentication policy resource Oct 25, 2024
@sfc-gh-jmichalak
Copy link
Collaborator

/ok-to-test sha=30ac8454e73a35afe30cea38fefe0fc4bfd0442e

Copy link

Integration tests failure for 30ac8454e73a35afe30cea38fefe0fc4bfd0442e

1 similar comment
Copy link

Integration tests failure for 30ac8454e73a35afe30cea38fefe0fc4bfd0442e

@sfc-gh-jmichalak sfc-gh-jmichalak merged commit ddea819 into Snowflake-Labs:main Oct 25, 2024
7 of 9 checks passed
sfc-gh-jmichalak pushed a commit that referenced this pull request Nov 8, 2024
##
[0.98.0](v0.97.0...v0.98.0)
(2024-11-08)

Feature scope readiness for V1:
[link](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/v1-preparations/ESSENTIAL_GA_OBJECTS.MD)
([Roadmap
reference](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#wrap-up-the-functional-scope)).
:exclamation: Migration guide: [v0.97.0 ->
v0.98.0](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#v0970--v0980)

### 🎉 What's new
- New resources:
- authentication_policy
([#3098](#3098)),
references
[#2880](#2880)
- external_volume
([#3106](#3106)),
partially references
[#2980](#2980)
- stream_on_directory_table
([#3129](#3129))
- stream_on_view
([#3150](#3150))
- primary_connection, secondary_connection
([#3162](#3162))
- secret_with_basic_authentication, secret_with_generic_string,
secret_with_oauth_authorization_code_grant,
secret_with_oauth_client_credentials
([#3110](#3110)),
([#3141](#3141))
- New data sources:
- connections
([#3155](#3155)),
([#3173](#3173))
- secrets
([#3131](#3131))
- Reworked:
- provider configuration hierarchy
([#3166](#3166)),
references
[#1881](#1881),
[#2145](#2145),
[#2925](#2925),
[#2983](#2983),
[#3104](#3104)
- provider configuration fields
([#3152](#3152))
streams data source
([#3151](#3151))
- SDK upgrades:
- Upgrade tag SDK
([#3126](#3126))
- Recreate streams when they are stale
([#3129](#3129))
### 🔧  Misc
- Add object renaming research summary
([#3172](#3172))
- Test support for object renaming
([#3130](#3130)),
([#3147](#3147)),
([#3154](#3154))
- Add tests to issue
[#3117](#3117)
([#3133](#3133))
- New roadmap entry
([#3158](#3158))
- Test more authentication methods
([#3178](#3178))
- Minor fixes
([#3174](#3174))
### 🐛  Bug fixes
- Apply various fixes
([#3176](#3176)),
this addresses BCR 2024_08, references
[#2717](#2717),
[#3005](#3005),
[#3125](#3125),
[#3127](#3127),
[#3153](#3153)
- Connection and secret data sources tests
([#3177](#3177))
- Fix grant import docs
([#3183](#3183)),
resolves
[#3179](https://github.com/Snowflake-Labs/terraform-provider-snowflake/discussions/3179)
- Fix user resource import
([#3181](#3181))
- Handle external type changes in stream resources
([#3164](#3164))
- Do not use OR REPLACE on initial creation in resources with
copy_grants
([#3129](#3129))
- Address issue
[#2201](#2201)
by introducing new stream resources

Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants