-
Notifications
You must be signed in to change notification settings - Fork 418
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
Add authentication policy resource #3098
base: main
Are you sure you want to change the base?
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.
"snowflake_account_parameter": resources.AccountParameter(), | ||
"snowflake_alert": resources.Alert(), | ||
"snowflake_account": resources.Account(), | ||
"snowflake_account_authentication_policy_attachment": resources.AccountAuthenticationPolicyAttachment(), |
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 don't think we need these attachment resources - we should attach directly in users and accounts 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.
You mean similar to the network policy attachment? I have taken the password policy resource and its attachment as an example.
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.
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
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 How shall I procede here?
) | ||
|
||
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)"?
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. |
Type: schema.TypeSet, | ||
Elem: &schema.Schema{ | ||
Type: schema.TypeString, | ||
ValidateDiagFunc: StringInSlice([]string{"ALL", "SAML", "PASSWORD", "OAUTH", "KEYPAIR"}, false), |
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.
Yes, please check out warehouse enums in sdk - To...
functions.
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)} |
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.
After implementing To...
enum validators, they should be used in all places when we convert from strings in user inputs to SDK fields.
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.
done, please check
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 { |
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.
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
}
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.
Done
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 👋 |
Added the following resources:
Test Plan
References