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

[Functions] Mask sensitive data on store_function #3096

Merged

Conversation

Tankilevitch
Copy link
Contributor

@Tankilevitch Tankilevitch commented Feb 15, 2023

Phase 2 of - #3076
Related to https://jira.iguazeng.com/browse/ML-3156

Multiple adjustments had to be done:

  • On store function masking credentials if auth_info is provided (will be done on store_function endpoint)
  • Changed the concatenation of the feature store merger to - instead of _ to align with the mlrun function naming conventions.
  • Removed functions from tests_dbs and removed all tests of runtime base which are no longer relevant as we are no longer support running runtime base as well as storing unstructured functions.

@Tankilevitch Tankilevitch force-pushed the mask-credentials-on-store-function branch from 71d34c0 to c78e1c8 Compare February 16, 2023 09:52
Copy link
Member

@theSaarco theSaarco left a comment

Choose a reason for hiding this comment

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

Looks good in general. Have some comments and suggestions...

Comment on lines 299 to 313
If access key is not mask (starts with secret prefix) then fill $generate so that the API will handle filling
of the credentials.
We rely on the HTTPDB to send the access key session through the request header and that the API will mask
the access key, that way we won't even store any plain access key in the function.
"""
if self.metadata.credentials.access_key and (
# if contains secret reference or $generate then no need to overwrite the access key
self.metadata.credentials.access_key.startswith(
mlrun.model.Credentials.secret_reference_prefix
)
or self.metadata.credentials.access_key.startswith(
mlrun.model.Credentials.generate_access_key
)
):
return
Copy link
Member

Choose a reason for hiding this comment

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

This is already merged - should probably rebase your changes.

return mlrun.api.utils.singletons.db.get_db().store_function(
db_session,
function,
name,
function_obj.to_dict(),
Copy link
Member

Choose a reason for hiding this comment

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

In case there's no auth_info you do a new_function just to do to_dict() on it... A lot of effort for nothing. I guess instead you can do the new_function -> apply_enrichment -> to_dict inside the if checking for auth_info.

tests/system/runtimes/test_kubejob.py Show resolved Hide resolved
Copy link
Member

@alonmr alonmr left a comment

Choose a reason for hiding this comment

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

LGTM. Left a question and a suggestion (Other than Saar's comments)

mlrun/runtimes/pod.py Show resolved Hide resolved
@Tankilevitch Tankilevitch marked this pull request as ready for review February 20, 2023 12:44
Copy link
Member

@theSaarco theSaarco left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants