Skip to content

Commit

Permalink
Add/Fix docs and previous change requests
Browse files Browse the repository at this point in the history
  • Loading branch information
sfc-gh-jcieslak committed Jul 11, 2024
1 parent 5b63c62 commit 01142bf
Show file tree
Hide file tree
Showing 7 changed files with 127 additions and 48 deletions.
58 changes: 58 additions & 0 deletions CREATING_ISSUES.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
* [How to set up the connection with the private key?](#how-to-set-up-the-connection-with-the-private-key)
* [Incorrect identifier (index out of bounds) (even with the old error message)](#incorrect-identifier-index-out-of-bounds-even-with-the-old-error-message)
* [Incorrect account identifier (snowflake_database.from_share)](#incorrect-account-identifier-snowflake_databasefrom_share)
* [Granting on Functions or Procedures](#granting-on-functions-or-procedures)
* [Infinite diffs, empty privileges, errors when revoking on grant resources](#infinite-diffs-empty-privileges-errors-when-revoking-on-grant-resources)

This guide was made to aid with creating the GitHub issues, so you can maximize your chances of getting help as quickly as possible.
To correctly report the issue, we suggest going through the following steps.
Expand Down Expand Up @@ -161,3 +163,59 @@ panic: interface conversion: sdk.ObjectIdentifier is sdk.AccountObjectIdentifier
[GitHub issue reference](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2590)

**Solution:** As specified in the [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#behavior-change-external-object-identifier-changes), use account locator instead.

### Granting on Functions or Procedures
**Problem:** Right now, when granting any privilege on Function or Procedure with this or similar configuration:

```terraform
resource "snowflake_grant_privileges_to_account_role" "grant_on_procedure" {
privileges = ["USAGE"]
account_role_name = snowflake_account_role.name
on_schema_object {
object_type = "PROCEDURE"
object_name = "\"${snowflake_database.database.name}\".\"${snowflake_schema.schema.name}\".\"${snowflake_procedure.procedure.name}\""
}
}
```

You may encounter the following error:
```text
│ Error: 090208 (42601): Argument types of function 'procedure_name' must be
│ specified.
```

**Related issues:** [#2375](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2375), [#2922](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2922)

**Solution:** Specify the arguments in the `object_name`:

```terraform
resource "snowflake_grant_privileges_to_account_role" "grant_on_procedure" {
privileges = ["USAGE"]
account_role_name = snowflake_account_role.name
on_schema_object {
object_type = "PROCEDURE"
object_name = "\"${snowflake_database.database.name}\".\"${snowflake_schema.schema.name}\".\"${snowflake_procedure.procedure.name}(NUMBER, VARCHAR)\""
}
}
```

### Infinite diffs, empty privileges, errors when revoking on grant resources
**Problem:** If you encountered one of the following issues:
- Issue with revoking: `Error: An error occurred when revoking privileges from an account role.
- Plan in every `terraform plan` run (mostly empty privileges)
It's possible that the `object_type` you specified is "incorrect."
Let's say you would like to grant `SELECT` on event table. In Snowflake, it's possible to specify
`TABLE` object type instead of dedicated `EVENT TABLE` one. As `object_type` is one of the fields
we filter on, it needs to exactly match with the output provided by `SHOW GRANTS` command.

**Related issues:** [#2749](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2749), [#2803](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2803)

**Solution:** Here's a list of things that may help with your issue:
- Firstly, check if the privilege has been granted in Snowflake. If it is, it means the configuration is correct (or at least compliant with Snowflake syntax).
- When granting `IMPORTED PRIVILEGES` on `SNOWFLAKE` database/application, use `object_type = "DATABASE"`.
- Run `SHOW GRANTS` command with the right filters to find the granted privilege and check what is the object type returned of that command. If it doesn't match the one you have in your configuration, then follow those steps:
- Use state manipulation (no revoking)
- Remove the resource from your state using `terraform state rm`.
- Change the `object_type` to correct value.
- Import the state from Snowflake using `terraform import`.
- Remove the grant configuration and after `terraform apply` put it back with the correct `object_type` (requires revoking).
4 changes: 2 additions & 2 deletions docs/technical-documentation/resource_migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ import {
}
```

If you are using terraform 1.7 or above you could use a `for_each` to import multiple resources at once. See
[Terraform 1.7 adds test mocking and config-driven remove > Import block for_each](https://www.hashicorp.com/blog/terraform-1-7-adds-test-mocking-and-config-driven-remove).
If you are using terraform 1.7 or above you could use a `for_each` to import multiple resources at once.
For more details, take a look at [importing multiple instances with for_each](https://developer.hashicorp.com/terraform/language/v1.7.x/import?product_intent=terraform#import-multiple-instances-with-for_each).

[Hashicorp documentation reference on import block](https://developer.hashicorp.com/terraform/language/import)

Expand Down
9 changes: 8 additions & 1 deletion examples/provider/provider.tf
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
terraform {
required_providers {
snowflake = {
source = "Snowflake-Labs/snowflake"
}
}
}

provider "snowflake" {
account = "..." # required if not using profile. Can also be set via SNOWFLAKE_ACCOUNT env var
username = "..." # required if not using profile or token. Can also be set via SNOWFLAKE_USER env var
Expand All @@ -23,7 +31,6 @@ provider "snowflake" {
}
}


provider "snowflake" {
profile = "securityadmin"
}
43 changes: 22 additions & 21 deletions pkg/resources/saml2_integration_acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package resources_test

import (
"fmt"
r "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/resources"
"maps"
"regexp"
"strings"
Expand Down Expand Up @@ -78,17 +79,17 @@ func TestAcc_Saml2Integration_basic(t *testing.T) {
ConfigVariables: m(issuer, string(sdk.Saml2SecurityIntegrationSaml2ProviderCustom), validUrl, cert, false, false),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "name", id.Name()),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "enabled", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "enabled", r.BooleanDefault),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_issuer", issuer),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_sso_url", validUrl),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_provider", string(sdk.Saml2SecurityIntegrationSaml2ProviderCustom)),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_x509_cert", cert),
resource.TestCheckNoResourceAttr("snowflake_saml2_integration.test", "saml2_sp_initiated_login_page_label"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_enable_sp_initiated", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_sign_request", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_enable_sp_initiated", r.BooleanDefault),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_sign_request", r.BooleanDefault),
resource.TestCheckNoResourceAttr("snowflake_saml2_integration.test", "saml2_requested_nameid_format"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_post_logout_redirect_url", ""),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", r.BooleanDefault),
resource.TestCheckNoResourceAttr("snowflake_saml2_integration.test", "saml2_snowflake_issuer_url"),
resource.TestCheckNoResourceAttr("snowflake_saml2_integration.test", "saml2_snowflake_acs_url"),
resource.TestCheckNoResourceAttr("snowflake_saml2_integration.test", "allowed_user_domains"),
Expand Down Expand Up @@ -316,17 +317,17 @@ func TestAcc_Saml2Integration_basic(t *testing.T) {
ConfigVariables: m(issuer, string(sdk.Saml2SecurityIntegrationSaml2ProviderCustom), validUrl, cert, false, true),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "name", id.Name()),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "enabled", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "enabled", r.BooleanDefault),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_issuer", issuer),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_sso_url", validUrl),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_provider", string(sdk.Saml2SecurityIntegrationSaml2ProviderCustom)),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_x509_cert", cert),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_sp_initiated_login_page_label", "foo"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_enable_sp_initiated", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_sign_request", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_enable_sp_initiated", r.BooleanDefault),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_sign_request", r.BooleanDefault),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_requested_nameid_format", ""),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_post_logout_redirect_url", ""),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", r.BooleanDefault),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_snowflake_issuer_url", issuerURL),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_snowflake_acs_url", acsURL),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "allowed_user_domains.#", "1"),
Expand Down Expand Up @@ -465,12 +466,12 @@ func TestAcc_Saml2Integration_forceAuthn(t *testing.T) {
PreApply: []plancheck.PlanCheck{
plancheck.ExpectResourceAction("snowflake_saml2_integration.test", plancheck.ResourceActionUpdate),
planchecks.PrintPlanDetails("snowflake_saml2_integration.test", "saml2_force_authn", "describe_output"),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String("true"), sdk.String("default")),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String("true"), sdk.String(r.BooleanDefault)),
planchecks.ExpectComputed("snowflake_saml2_integration.test", "describe_output", true),
},
},
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", r.BooleanDefault),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.#", "1"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.0.saml2_force_authn.0.value", "false"),
),
Expand All @@ -480,7 +481,7 @@ func TestAcc_Saml2Integration_forceAuthn(t *testing.T) {
ConfigPlanChecks: resource.ConfigPlanChecks{
PreApply: []plancheck.PlanCheck{
planchecks.PrintPlanDetails("snowflake_saml2_integration.test", "saml2_force_authn", "describe_output"),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String("default"), sdk.String("default")),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String(r.BooleanDefault), sdk.String(r.BooleanDefault)),
planchecks.ExpectComputed("snowflake_saml2_integration.test", "describe_output", true),
},
},
Expand All @@ -501,12 +502,12 @@ func TestAcc_Saml2Integration_forceAuthn(t *testing.T) {
PreApply: []plancheck.PlanCheck{
plancheck.ExpectNonEmptyPlan(),
planchecks.PrintPlanDetails("snowflake_saml2_integration.test", "saml2_force_authn", "describe_output"),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String("false"), sdk.String("default")),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String("false"), sdk.String(r.BooleanDefault)),
planchecks.ExpectComputed("snowflake_saml2_integration.test", "describe_output", true),
},
},
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", r.BooleanDefault),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.#", "1"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.0.saml2_force_authn.0.value", "false"),
),
Expand All @@ -522,12 +523,12 @@ func TestAcc_Saml2Integration_forceAuthn(t *testing.T) {
PreApply: []plancheck.PlanCheck{
plancheck.ExpectNonEmptyPlan(),
planchecks.PrintPlanDetails("snowflake_saml2_integration.test", "saml2_force_authn", "describe_output"),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String("true"), sdk.String("default")),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String("true"), sdk.String(r.BooleanDefault)),
planchecks.ExpectComputed("snowflake_saml2_integration.test", "describe_output", true),
},
},
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", r.BooleanDefault),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.#", "1"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.0.saml2_force_authn.0.value", "false"),
),
Expand Down Expand Up @@ -976,16 +977,16 @@ func TestAcc_Saml2Integration_DefaultValues(t *testing.T) {
ConfigVariables: configVariables,
ConfigPlanChecks: resource.ConfigPlanChecks{
PreApply: []plancheck.PlanCheck{
planchecks.ExpectChange("snowflake_saml2_integration.test", "enabled", tfjson.ActionUpdate, sdk.String("false"), sdk.String("default")),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String("false"), sdk.String("default")),
planchecks.ExpectChange("snowflake_saml2_integration.test", "enabled", tfjson.ActionUpdate, sdk.String("false"), sdk.String(r.BooleanDefault)),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String("false"), sdk.String(r.BooleanDefault)),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_post_logout_redirect_url", tfjson.ActionUpdate, sdk.String(""), sdk.String("")),
planchecks.ExpectComputed("snowflake_saml2_integration.test", "show_output", true),
planchecks.ExpectComputed("snowflake_saml2_integration.test", "describe_output", true),
},
},
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "enabled", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", "default"),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "enabled", r.BooleanDefault),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", r.BooleanDefault),
resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_post_logout_redirect_url", ""),

resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "show_output.#", "1"),
Expand All @@ -1002,8 +1003,8 @@ func TestAcc_Saml2Integration_DefaultValues(t *testing.T) {
ConfigVariables: configVariables,
ConfigPlanChecks: resource.ConfigPlanChecks{
PreApply: []plancheck.PlanCheck{
planchecks.ExpectChange("snowflake_saml2_integration.test", "enabled", tfjson.ActionUpdate, sdk.String("default"), sdk.String("default")),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String("default"), sdk.String("default")),
planchecks.ExpectChange("snowflake_saml2_integration.test", "enabled", tfjson.ActionUpdate, sdk.String(r.BooleanDefault), sdk.String(r.BooleanDefault)),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String(r.BooleanDefault), sdk.String(r.BooleanDefault)),
planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_post_logout_redirect_url", tfjson.ActionUpdate, sdk.String(""), sdk.String("")),
planchecks.ExpectComputed("snowflake_saml2_integration.test", "show_output", true),
planchecks.ExpectComputed("snowflake_saml2_integration.test", "describe_output", true),
Expand Down
2 changes: 1 addition & 1 deletion pkg/resources/table.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ var tableSchema = map[string]*schema.Schema{
"type": {
Type: schema.TypeString,
Required: true,
Description: "Column type, e.g. VARIANT",
Description: "Column type, e.g. VARIANT. For a full list of column types, see [Summary of Data Types](https://docs.snowflake.com/en/sql-reference/intro-summary-data-types).",
ValidateFunc: dataTypeValidateFunc,
DiffSuppressFunc: dataTypeDiffSuppressFunc,
},
Expand Down
13 changes: 13 additions & 0 deletions v1-preparations/CHANGES_BEFORE_V1.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,16 @@ Because of the changes regarding [Config values in the state](#config-values-in-
- `parameters` computed field, containing all the values and levels of Snowflake parameters (the result of `SHOW PARAMETERS IN <object> <name>`)

This way, it is still possible to obtain the values in your configs, even without setting them directly for the given managed object.

## Object cloning
Some of the Snowflake objects (like [Database](https://docs.snowflake.com/en/sql-reference/sql/create-database)) can create clones from already existing objects.
For now, we decided to drop the support for object cloning. That's why during the [resource preparation for V1](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#preparing-essential-ga-objects-for-the-provider-v1)
we will be removing the option to clone (if they exist in the current implementation). The main reasons behind that decision are
- With [object cloning](https://docs.snowflake.com/en/user-guide/object-clone) we have to keep in mind additional things that still have to be researched to check how they're matching within the Terraform ecosystem.
- There is potentially not enough information in Snowflake available for the end users (like us) to track the information about the connection between cloned objects.
- There are still at least a few of the design decisions that have to be answered before knowingly putting cloning into resources

Because of that, we would like to shelve the idea of introducing cloning to the resources (at least for V1). After V1,
object cloning is one of the topics we would like to take a closer look at. Right now, the cloning can be done manually
and imported into normal resources, but in case there is any divergence between the normal and cloned object, the resources
may act in an unexpected way. An alternative solution is to use plain SQL with [unsafe execute resources](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs/resources/unsafe_execute) for now.
Loading

0 comments on commit 01142bf

Please sign in to comment.