-
Notifications
You must be signed in to change notification settings - Fork 427
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
feat: Add authentication policy resource #3098
Conversation
@sfc-gh-jcieslak 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?
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? |
Hi! Thanks for this PR, it is a really important one for my team :) |
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 I think this should be added as a part of the |
Hey @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
Outdated
Show resolved
Hide resolved
"snowflake_account_password_policy_attachment": resources.AccountPasswordPolicyAttachment(), | ||
"snowflake_account_parameter": resources.AccountParameter(), | ||
"snowflake_alert": resources.Alert(), | ||
"snowflake_account": resources.Account(), |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
if err = d.Set("database", authenticationPolicy.DatabaseName); err != nil { | ||
return diag.FromErr(err) | ||
} | ||
if err = d.Set("schema", authenticationPolicy.SchemaName); err != nil { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- When changed in the configuration, the Terraform detects this change anyway (and sets it in the state).
- 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() |
There was a problem hiding this comment.
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
- 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.
There was a problem hiding this comment.
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)"?
There was a problem hiding this comment.
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.
if err = d.Set("database", authenticationPolicy.DatabaseName); err != nil { | ||
return diag.FromErr(err) | ||
} | ||
if err = d.Set("schema", authenticationPolicy.SchemaName); err != nil { |
There was a problem hiding this comment.
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.
hi @sfc-gh-jmichalak @sfc-gh-jcieslak Is there progress related to this PR? |
Hey @csp33 |
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. |
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. |
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)
bc8534a
to
a2f467a
Compare
@sfc-gh-jcieslak @sfc-gh-jmichalak
where ["ALL"] is the Snowflake default for this parameter. However, setting the PS: It works now, after I have added a custom check to
PS2: The unwanted consequence of the "hack" in
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. |
Hey @Relativity74205 👋 |
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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:
- When changed in the configuration, the Terraform detects this change anyway (and sets it in the state).
- 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
pkg/resources/account_authentication_policy_attachment_acceptance_test.go
Outdated
Show resolved
Hide resolved
@sfc-gh-jmichalak |
/ok-to-test sha=30ac8454e73a35afe30cea38fefe0fc4bfd0442e |
Integration tests failure for 30ac8454e73a35afe30cea38fefe0fc4bfd0442e |
1 similar comment
Integration tests failure for 30ac8454e73a35afe30cea38fefe0fc4bfd0442e |
## [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>
Added the following resources:
Test Plan
References