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

incorrect order of precedence for SNOWFLAKE_ACCOUNT variable #2294

Closed
coleheflin-gt opened this issue Dec 21, 2023 · 7 comments
Closed

incorrect order of precedence for SNOWFLAKE_ACCOUNT variable #2294

coleheflin-gt opened this issue Dec 21, 2023 · 7 comments
Labels
bug Used to mark issues with provider's incorrect behavior

Comments

@coleheflin-gt
Copy link

Terraform CLI and Provider Versions

Terraform v1.6.1
Snowflake Labs 0.80.0

Terraform Configuration

terraform {

  required_version = ">= 1.6.1"

  required_providers {
    snowflake = {
      source  = "Snowflake-Labs/snowflake"
      version = "0.80.0"
    }
  }
}

provider "snowflake" {
  account       = local.snowflake_account
  authenticator = "JWT"
}

provider "snowflake" {
  alias         = "account_admin"
  role          = "ACCOUNTADMIN"
  account       = local.snowflake_account
  authenticator = "JWT"
}

provider "snowflake" {
  alias         = "security_admin"
  role          = "SECURITYADMIN"
  account       = local.snowflake_account
  authenticator = "JWT"
}

provider "snowflake" {
  alias         = "sys_admin"
  role          = "SYSADMIN"
  account       = local.snowflake_account
  authenticator = "JWT"
}

Expected Behavior

Based on the terraform docs (scroll to the bottom), "The Snowflake provider will use the following order of precedence when determining which credentials to use: 1) Provider Configuration 2) Environment Variables 3) Config File".

We are expecting the local.snowflake_account variable to be used by the snowflake provider.

Actual Behavior

We are passing in the snowflake account directly into the provider and we also have an environment variable called SNOWFLAKE_ACCOUNT. The account passed directly into the provider is being overwritten and the value in the SNOWFLAKE_ACCOUNT environment variable is being used.

Steps to Reproduce

  1. Set an env variable SNOWFLAKE_ACCOUNT to an account
  2. Pass a different snowflake account into the provider (you may need to specify JWT auth, this wasn't happening before we switched to it)
  3. run a plan and check which account the queries go to

How much impact is this issue causing?

High

Logs

No response

Additional Information

No response

@coleheflin-gt coleheflin-gt added the bug Used to mark issues with provider's incorrect behavior label Dec 21, 2023
@coleheflin-gt coleheflin-gt changed the title SNOWFLAKE_ACCOUNT order of precedence SNOWFLAKE_ACCOUNT incorrect order of precedence Dec 21, 2023
@coleheflin-gt coleheflin-gt changed the title SNOWFLAKE_ACCOUNT incorrect order of precedence incorrect order of precedence for SNOWFLAKE_ACCOUNT variable Dec 21, 2023
@sfc-gh-asawicki
Copy link
Collaborator

Hey @coleheflin-gt. Thanks for reporting the issue.

This is still a blowback after unfortunate #2126. BTW, the documentation needed to be corrected before (now and before the refactor documentation was wrong).

I will talk with the team. We have to decide:

  • what precedence do we ultimately want (is the one in the docs the desired one)?
  • When can we make the change so as not to break the existing configurations?

I will get back to you as soon as we have any conclusions.

@coleheflin-gt
Copy link
Author

@sfc-gh-asawicki Thanks for the quick response, that makes sense!

My preference is to maintain the existing order of precedence outlined in the documentation. When we explicitly pass a value into a provider it should take precedence otherwise it will lead to confusion of where that value is being set. In our scenario, the production snowflake account value was set as an environment variable (for a different repository not related to snowflake terraform work) and I was on the terraform staging workspace expecting to be making changes in staging (since the staging account is passed into the provider explicitly) but I was running tf plans in production instead. Luckily the build was broken due to some changes so I never actually applied changes in production (when I thought I was in staging).

To minimize breaking changes, perhaps your team could implement logging when a plan/apply occurs that displays which account it is set to and where it got it from.

I realize you have to take into account many team's use cases, but I hope the described situation is helpful in your team coming to a decision! Thanks for the support on this.

@AZenat
Copy link

AZenat commented Feb 18, 2024

It actually impacts all environment variables, not only SNOWFLAKE_ACCOUNT.

@sfc-gh-asawicki
Copy link
Collaborator

Hey @AZenat @coleheflin-gt. We will work on this issue this week. I hope it will land with the next week's release. I will keep you posted.

sfc-gh-asawicki added a commit that referenced this issue Feb 23, 2024
Before fixing #2294 and #2242 we need to have a clean CI env. After this
PR it should be possible to completely remove following environment
variables from the secrets:
- AWS_EXTERNAL_BUCKET_URL (it has TEST_SF_TF_AWS_EXTERNAL_BUCKET_URL
replacement)
- AWS_EXTERNAL_KEY_ID (it has TEST_SF_TF_AWS_EXTERNAL_KEY_ID
replacement)
- AWS_EXTERNAL_ROLE_ARN (it has TEST_SF_TF_AWS_EXTERNAL_ROLE_ARN
replacement)
- AWS_EXTERNAL_SECRET_KEY (it has TEST_SF_TF_AWS_EXTERNAL_SECRET_KEY
replacement)
- AZURE_EXTERNAL_BUCKET_URL (it has TEST_SF_TF_AZURE_EXTERNAL_BUCKET_URL
replacement)
- AZURE_EXTERNAL_SAS_TOKEN (it has TEST_SF_TF_AZURE_EXTERNAL_SAS_TOKEN
replacement)
- AZURE_EXTERNAL_TENANT_ID (it has TEST_SF_TF_AZURE_EXTERNAL_TENANT_ID
replacement)
- GCS_EXTERNAL_BUCKET_URL (it has TEST_SF_TF_GCS_EXTERNAL_BUCKET_URL
replacement)
- SKIP_EMAIL_INTEGRATION_TESTS (not used)
- SKIP_EXTERNAL_TABLE_TEST (not used anymore)
- SKIP_NOTIFICATION_INTEGRATION_TESTS (not used)
- SKIP_STREAM_TEST (not used anymore)
- SNOWFLAKE_ACCOUNT (we use the config now, this shouldn't be needed)
- SNOWFLAKE_ACCOUNT_SECOND (not used anymore)
- SNOWFLAKE_ACCOUNT_THIRD (not used anymore)
- SNOWFLAKE_PASSWORD (we use the config now, this shouldn't be needed)
- SNOWFLAKE_ROLE (we use the config now, this shouldn't be needed)
- SNOWFLAKE_USER (we use the config now, this shouldn't be needed)

There are three more that should be removed but they should be
approached more carefully:
- SNOWFLAKE_WAREHOUSE (we do not have it in our config, so it is taken
from env; test what happens without setting a warehouse by running the
tests locally with the config similar to the CI one)
- SNOWFLAKE_PORT (we do not have this in our config, let's check if the
tests will work without this - they should not fail)
- SNOWFLAKE_PROTOCOL (we do not have this in our config, there is a
default, it should be fine without it)
sfc-gh-asawicki added a commit that referenced this issue Feb 23, 2024
sfc-gh-asawicki added a commit that referenced this issue Feb 26, 2024
Fix the order of precedence for the provider's config:
- removed fallback to environment variables for the SDK client (we will
address the setup of the client separately with the extraction of the
SDK client from this repository)
- fixed the merge config function to fill out only missing parameters
- added missing tests
- tested the provider setup in an acceptance test
- introduced environment helpers with tests
- started to move env to one place (the rest envs can be moved after
discussing in this PR if we all agree with that direction)
- added migration notes

References: #2242 #2294
@sfc-gh-asawicki
Copy link
Collaborator

Hey @AZenat @coleheflin-gt. We have released the fix as part of v0.87.0 release. Please follow the migration guide during the update. Please confirm that the issue is resolved in the newest version. Thanks!

@coleheflin-gt
Copy link
Author

@sfc-gh-asawicki Thanks for the update. We upgraded to the 0.87.0 and it works as expected! The issue has been resolved. Thanks for the work on this.

@sfc-gh-asawicki
Copy link
Collaborator

Great! I'm closing the issue then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to mark issues with provider's incorrect behavior
Projects
None yet
Development

No branches or pull requests

3 participants