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

Issue with the SnowflakeEncryptedPrivateKeyPemProfileMapping #632

Closed
DanMawdsleyBA opened this issue Oct 27, 2023 · 16 comments · Fixed by #649
Closed

Issue with the SnowflakeEncryptedPrivateKeyPemProfileMapping #632

DanMawdsleyBA opened this issue Oct 27, 2023 · 16 comments · Fixed by #649
Labels
bug Something isn't working profile:snowflake Related to Snowflake ProfileConfig
Milestone

Comments

@DanMawdsleyBA
Copy link
Contributor

image

With the new snowflake connection with an encrypted private key in the creation of the profile it misses out the "private_key" like how it does in in the non encrypted version.

@tatiana
Copy link
Collaborator

tatiana commented Oct 27, 2023

Hey @DanMawdsleyBA ! Just confirming, this is in 1.3.0a1, right?

cc @ivanstillfront

@tatiana tatiana added this to the 1.3.0 milestone Oct 27, 2023
@DanMawdsleyBA
Copy link
Contributor Author

DanMawdsleyBA commented Oct 27, 2023

Yeah part of 1.3.0a1. For context this is the profile for the non encrypted one:
image

I would expect the encrypted one to have both the private_key and private_key_passphrase

@DanMawdsleyBA
Copy link
Contributor Author

Looking again the current method uses the private key path but I am looking to use just the private key. I think the name may be confusing as we may need two methods for encrypted snowflake access:
private_key_path+passphrase
private_key+passphrase

Renaming them to something like the below might make sense although would mean a breaking change from the early release

  • SnowflakeEncryptedPrivateKeyFilePemProfileMapping
  • SnowflakeEncryptedPrivateKeyPemProfileMapping

Unless there is a way to combine them into one

@tatiana Any thoughts on how to best manage the above?

@tatiana
Copy link
Collaborator

tatiana commented Nov 2, 2023

@DanMawdsleyBA, well observed! I like your suggested names - we expose them in a new version and add a deprecation warning to the previous class, WDYT? This way, we won't be breaking users who may have adopted it, but we'll be encouraging and documenting the new way.

Would you, by any chance, be able to contribute to this?

@mattxxi
Copy link

mattxxi commented Nov 2, 2023

Hey :)
I jump on this topic because on my case I can't use cosmos because the only way to access our snowflake is via key / pair encrypted.

In documentation I found this method: https://astronomer.github.io/astronomer-cosmos/profiles/SnowflakeEncryptedPrivateKeyPem.html
which actually does not exists anymore in the last version.

Here the DBT profile I need to set up to make it work:
default:
outputs:
dev:
account:
database:
private_key_passphrase:
private_key_path:
role:
schema:
threads:
type: snowflake
user:
warehouse:
target: dev

Would it be possible to add the profile with encryption back?

Thanks :)

@DanMawdsleyBA
Copy link
Contributor Author

Hey :) I jump on this topic because on my case I can't use cosmos because the only way to access our snowflake is via key / pair encrypted.

In documentation I found this method: https://astronomer.github.io/astronomer-cosmos/profiles/SnowflakeEncryptedPrivateKeyPem.html which actually does not exists anymore in the last version.

Here the DBT profile I need to set up to make it work: default: outputs: dev: account: database: private_key_passphrase: private_key_path: role: schema: threads: type: snowflake user: warehouse: target: dev

Would it be possible to add the profile with encryption back?

Thanks :)

So the documentation is from the pre-release version (Confused me as well at first) If you update to the 1.3.0a1 version you should see it. Although the name may change from the pre-release version to allow for 2 different methods :
private_key_path+passphrase
private_key+passphrase

So for your case a new profile mapping:
SnowflakeEncryptedPrivateKeyFilePemProfileMapping ->private_key_path+passphrase
In my case I would use:
SnowflakeEncryptedPrivateKeyPemProfileMapping ->private_key+passphrase

@tatiana just checking you're happy with that approach. I will do my best to find some time do you have a planned 1.3 release date?

@mattxxi
Copy link

mattxxi commented Nov 2, 2023

Okay thanks, I found a workaround to make it work while the release comes out:

from airflow.hooks.base import BaseHook
from cosmos.profiles import SnowflakePrivateKeyPemProfileMapping

# Get the connection of snowflake
cnx = BaseHook.get_connection("snowflake_conn")

profile_mapping = SnowflakePrivateKeyPemProfileMapping(
    conn_id="snowflake_conn",
    profile_args={
        "private_key_passphrase": cnx.get_password()
    }
)

Note:
Also could be nice to specify the format for the private key --> text without begin & end.

@ivanstillfront
Copy link
Contributor

Why even use these mapping classes?

Cosmos has a utility function get_automatic_profile_mapping that works way better and is future-proof in case the mapping class is deprecated:

from cosmos import ProfileConfig
from cosmos.profiles import get_automatic_profile_mapping

profile_mapping = get_automatic_profile_mapping(conn_id="snowflake_conn")

profile_config = ProfileConfig(
  profile_name="profile_name",
  profile_mapping=profile_mapping[0],
)

@DanMawdsleyBA
Copy link
Contributor Author

@ivanstillfront I think the automated one choose the correct profile mapping behind the scenes so it would still need the profile mapping in the first place.

I've tried @mattxxi workaround but I am getting an issue I think something to do with how the key gets decrypted error below:
image
I use the connection in airflow for connecting to snowflake which works fine elsewhere. So might be a separate issue with either cosmos/dbt.

Saw this not sure if this is related to the key profile mapping:
dbt-labs/dbt-snowflake#671

@jlaneve
Copy link
Collaborator

jlaneve commented Nov 3, 2023

Why even use these mapping classes?

Cosmos has a utility function get_automatic_profile_mapping that works way better and is future-proof in case the mapping class is deprecated:

from cosmos import ProfileConfig
from cosmos.profiles import get_automatic_profile_mapping

profile_mapping = get_automatic_profile_mapping(conn_id="snowflake_conn")

profile_config = ProfileConfig(
  profile_name="profile_name",
  profile_mapping=profile_mapping[0],
)

It's a valid question. The big reason is that using get_automatic_profile_mapping requires you to have the connection defined. This means that, for example, in CI/CD you need your live connections defined. We've had a lot of users run into issues with that, so the recommendation today is to pick the mapping you know you're going to use unless you don't need your DAGs to parse in CI/CD and are okay with Airflow reaching out to the metadata DB to get the connection on DAG parse time.

@ivanstillfront
Copy link
Contributor

Why even use these mapping classes?
Cosmos has a utility function get_automatic_profile_mapping that works way better and is future-proof in case the mapping class is deprecated:

from cosmos import ProfileConfig
from cosmos.profiles import get_automatic_profile_mapping

profile_mapping = get_automatic_profile_mapping(conn_id="snowflake_conn")

profile_config = ProfileConfig(
  profile_name="profile_name",
  profile_mapping=profile_mapping[0],
)

It's a valid question. The big reason is that using get_automatic_profile_mapping requires you to have the connection defined. This means that, for example, in CI/CD you need your live connections defined. We've had a lot of users run into issues with that, so the recommendation today is to pick the mapping you know you're going to use unless you don't need your DAGs to parse in CI/CD and are okay with Airflow reaching out to the metadata DB to get the connection on DAG parse time.

@jlaneve Airflow recommends using env vars to mock connections in ci/cd pipelines, see: https://airflow.apache.org/docs/apache-airflow/2.0.1/best-practices.html#mocking-variables-and-connections

@tatiana
Copy link
Collaborator

tatiana commented Nov 7, 2023

Hi @DanMawdsleyBA , sorry for the delay getting back to you - thank you very much for the PR! Yes, I'm happy with the approach.

The profile mapping documentation is automated and unfortunately, we don't have versioned documentation yet. So, yes, I'm sorry for the confusion.

We don't yet have a date for the 1.3 release - but we could release 1.3.0a2 with these changes, so you are not blocked, would that be fine?

@DanMawdsleyBA
Copy link
Contributor Author

DanMawdsleyBA commented Nov 7, 2023

Yeah that would be great thanks! FYI the other issue I had was due to the fact 1.5.5 didn't support environment variables using the snowflake PEM key type. This is fixed in dbt-snowflake 1.7
dbt-labs/dbt-snowflake#619

@tatiana tatiana added profile:snowflake Related to Snowflake ProfileConfig bug Something isn't working labels Nov 8, 2023
@tatiana tatiana closed this as completed in 2ec33b4 Nov 9, 2023
@DanMawdsleyBA
Copy link
Contributor Author

@tatiana Would it be possible to get an alpha release for this so I can test?

@tatiana
Copy link
Collaborator

tatiana commented Nov 23, 2023

@DanMawdsleyBA
Copy link
Contributor Author

That's great thanks! All seems to be working now :) Was great to meet you!

arojasb3 pushed a commit to arojasb3/astronomer-cosmos that referenced this issue Jul 14, 2024
…ronomer#649)

Adds a snowflake mapping for encrypted private key using an environment variable

Closes: astronomer#632

Breaking Change?
This does rename the previous SnowflakeEncryptedPrivateKeyFilePemProfileMapping to SnowflakeEncryptedPrivateKeyFilePemProfileMapping but this makes it clearer as a new SnowflakeEncryptedPrivateKeyPemProfileMapping is added which supports the env variable. Also was only released as a pre-release change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working profile:snowflake Related to Snowflake ProfileConfig
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants