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

feat: Allow feast snowflake to read in byte string for private-key authentication #4384

Merged

Conversation

arturkolakowski
Copy link
Contributor

What this PR does / why we need it:

Allows the user to pass private_key as a byte string for Snowflake key-pair authentication rather than only limiting the ability to refer to the file path where the private key is stored.
Gives the ability to pass in private_key from a Secret Manager service (i.e: Google Cloud Secret Manager)

Which issue(s) this PR fixes:

Fixes

…cation

Signed-off-by: Artur <artur.kolakowski@medely.com>
@arturkolakowski arturkolakowski changed the title feat: allow feast snowflake to read in byte string for private-key authentication feat: Allow feast snowflake to read in byte string for private-key authentication Aug 5, 2024
@tokoko
Copy link
Collaborator

tokoko commented Aug 7, 2024

just curious, are you able to set bytes value from yaml as well or will this only work in python?

Artur added 2 commits August 8, 2024 08:31
Signed-off-by: Artur <artur.kolakowski@medely.com>
Signed-off-by: Artur <artur.kolakowski@medely.com>
@arturkolakowski
Copy link
Contributor Author

just curious, are you able to set bytes value from yaml as well or will this only work in python?

yes it will work, for example, feature_store.yaml can include private_key of format

'-----BEGIN ENCRYPTED PRIVATE KEY-----
.
.
-----END ENCRYPTED PRIVATE KEY-----'

Signed-off-by: Artur <artur.kolakowski@medely.com>
@tokoko
Copy link
Collaborator

tokoko commented Aug 8, 2024

Wouldn't that be interpreted as a string and therefore as a file path?

@arturkolakowski
Copy link
Contributor Author

As I am looking through the code base and considering this example: SnowflakeOfflineStore
SnowflakeOfflineStore establishes a snowflake connection with GetSnowflakeConnection (/feast/infra/utils/snowflake/snowflake_utils.py)

with GetSnowflakeConnection(config.offline_store) as conn:
            snowflake_conn = conn

when parse_private_key_path gets called private_key is already of type bytes so this way only works in python, my apologies.

@tokoko
Copy link
Collaborator

tokoko commented Aug 12, 2024

@arturkolakowski how about we add another field in config instead (something like private_key_content) which would simply take precedence?

Artur added 2 commits August 12, 2024 15:58
…th by reading in byte string

Signed-off-by: Artur <artur.kolakowski@medely.com>
Signed-off-by: Artur <artur.kolakowski@medely.com>
@arturkolakowski
Copy link
Contributor Author

@tokoko Made changes based on the request

Artur added 2 commits August 12, 2024 17:12
Signed-off-by: Artur <artur.kolakowski@medely.com>
Signed-off-by: Artur <artur.kolakowski@medely.com>
Copy link
Collaborator

@tokoko tokoko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks

@tokoko tokoko merged commit 5215a21 into feast-dev:master Aug 13, 2024
17 checks passed
shuchu pushed a commit to shuchu/feast that referenced this pull request Aug 14, 2024
…thentication (feast-dev#4384)

* allow feast snowflake to read in byte string for private-key authentication

Signed-off-by: Artur <artur.kolakowski@medely.com>

* Update type hint for  to use Union instead of | syntax

Signed-off-by: Artur <artur.kolakowski@medely.com>

* Update type hint for private_key to use Union instead of | syntax

Signed-off-by: Artur <artur.kolakowski@medely.com>

* Update type hint in parse_private_key_path

Signed-off-by: Artur <artur.kolakowski@medely.com>

* added private_key_content in Snowflake configs to support key-pair auth by reading in byte string

Signed-off-by: Artur <artur.kolakowski@medely.com>

* fix incompatible linting types

Signed-off-by: Artur <artur.kolakowski@medely.com>

* remove unused Union import

Signed-off-by: Artur <artur.kolakowski@medely.com>

* fix formating

Signed-off-by: Artur <artur.kolakowski@medely.com>

---------

Signed-off-by: Artur <artur.kolakowski@medely.com>
Co-authored-by: Artur <artur.kolakowski@medely.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants